Good point about the unclean election behavior. I agree we wouldn't want to elect a replica was on a shutting down broker.
> Right. There is a note about this in the Compatibility section. Thanks :) I missed that -David On Thu, Jun 2, 2022 at 12:19 PM David Jacot <dja...@confluent.io.invalid> wrote: > Thanks for your feedback, David. Please find my answers below. > > > When an unclean election is performed, I'm assuming we will ignore the > two > new invariants proposed in the KIP? If so, maybe we should clarify that. > > I am not sure about this. What's the point of electing a broker which > is fenced or shutting down in this case? At the moment, I think that > we already prevent a fenced broker from being uncleanly elected if not > mistaken. The KIP just extends this to include shutting down brokers > as well. > > > I understand the use case of persisting the in-controller-shutdown state > for controller failover, but are we planning on using this state on the > leader side for ISR eligibility? Or will we just rely on the controller to > check this state. > > Yes. The KIP explains that the leader will also enforce this based on > the information it gets via the metadata log. The idea is to prevent > unnecessary AlterPartition requests to the controller. > > > I think we should bump the IBP/MetadataVersion since we are adding a new > field to a metadata record. Otherwise we might have trouble downgrading the > software if this field has been serialized. > > Right. There is a note about this in the Compatibility section. > > Best, > David > > On Thu, Jun 2, 2022 at 6:04 PM David Arthur <mum...@gmail.com> wrote: > > > > David, thanks for the KIP! > > > > When an unclean election is performed, I'm assuming we will ignore the > two > > new invariants proposed in the KIP? If so, maybe we should clarify that. > > > > I understand the use case of persisting the in-controller-shutdown state > > for controller failover, but are we planning on using this state on the > > leader side for ISR eligibility? Or will we just rely on the controller > to > > check this state. > > > > I think we should bump the IBP/MetadataVersion since we are adding a new > > field to a metadata record. Otherwise we might have trouble downgrading > the > > software if this field has been serialized. > > > > Cheers, > > David > > > > On Thu, Jun 2, 2022 at 11:04 AM David Jacot <dja...@confluent.io.invalid > > > > wrote: > > > > > Hi José, > > > > > > At the moment, the KIP stipulates that the broker remains in > > > InControlledShutdown state until it is re-registered with a new > > > incarnation id. This implies that a broker can be both fenced and in > > > controlled shutdown state. We could make them mutually exclusive but I > > > think that there is value in the current proposal because we are able > > > to differentiate if a broker was fenced due to the controlled shutdown > > > or not. > > > > > > Best, > > > David > > > > > > On Wed, Jun 1, 2022 at 7:32 PM José Armando García Sancio > > > <jsan...@confluent.io.invalid> wrote: > > > > > > > > Thanks for the updates to the KIP. > > > > > > > > I like enumerating invariants. Is it safe to say that if > > > > `InControlledShutdown` is true then `Fenced` must be false. > > > > > > > > > -- > > David Arthur > -- David Arthur
