Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
That's correct.
David
On Fri, Jun 3, 2022 at 2:11 AM José Armando García Sancio
wrote:
>
> David Jacot wrote:
> > 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.
>
> Thanks David. Is this the reason why the BrokerRegistrationChangeRecord says:
>
> > { "name": "InControlledShutdown", "type": "int8", "versions": "1+",
> > "taggedVersions": "1+", "tag": 1,
> > "about": "0 if no change, 1 if the broker is in controlled shutdown." }
>
> In other words the only way to change the InControlShutdown to false
> is to create a new registration with a new incarnation id.
>
> > The broker will leave this state when it registers itself with a new
> > incarnation id.
>
> -José
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
David Jacot wrote:
> 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.
Thanks David. Is this the reason why the BrokerRegistrationChangeRecord says:
> { "name": "InControlledShutdown", "type": "int8", "versions": "1+",
> "taggedVersions": "1+", "tag": 1,
> "about": "0 if no change, 1 if the broker is in controlled shutdown." }
In other words the only way to change the InControlShutdown to false
is to create a new registration with a new incarnation id.
> The broker will leave this state when it registers itself with a new
> incarnation id.
-José
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
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 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 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 > > > 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 > > > 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
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
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 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 > 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 > > 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
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
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 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 > 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
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
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 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.
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
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.
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi Ismael, That's the plan. I just noticed that I forgot to change the version of the field in the AlterPartitionResponse. Let me fix that. Thanks, David On Wed, Jun 1, 2022 at 10:40 AM Ismael Juma wrote: > > Hi David, > > If you are adding topic IDs, should we remove topic names? > > Ismael > > On Tue, May 31, 2022, 6:36 AM David Jacot > wrote: > > > Hi all, > > > > Thanks for your feedback. > > > > I just updated the KIP as follows: > > * I propose to add a `TopicId` field at the same time while we are bumping > > the AlterPartition API version. > > * I propose to add the `InShuttingDown` field to the registration records > > to > > track if a broker is in controlled shutdown. This is useful for debugging > > and > > it would also prevent unnecessary AlterPartition requests from getting sent > > to the controller by the leaders. This will also help for KAFKA-13944. > > > > Artem - That's a good question. As explained by Colin we don't have this > > notion of fenced replicas in the ZK world. > > > > José - That's right. I have updated that section as you suggested. > > > > Best, > > David > > > > On Tue, May 24, 2022 at 5:39 PM José Armando García Sancio > > wrote: > > > > > > Hi David, > > > > > > Thanks for the KIP. In the "Compatibility, Deprecation, and Migration > > > Plan", you have: > > > > > > > The change is backward compatible. > > > > > > I assume that we don't need to increase the metadata.version/IBP for > > > AlterPartition because AlterPartitionManager uses ApiVersions for that > > > channel. Should we mention that in that section? > > > > > > -- > > > -José > >
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi David, If you are adding topic IDs, should we remove topic names? Ismael On Tue, May 31, 2022, 6:36 AM David Jacot wrote: > Hi all, > > Thanks for your feedback. > > I just updated the KIP as follows: > * I propose to add a `TopicId` field at the same time while we are bumping > the AlterPartition API version. > * I propose to add the `InShuttingDown` field to the registration records > to > track if a broker is in controlled shutdown. This is useful for debugging > and > it would also prevent unnecessary AlterPartition requests from getting sent > to the controller by the leaders. This will also help for KAFKA-13944. > > Artem - That's a good question. As explained by Colin we don't have this > notion of fenced replicas in the ZK world. > > José - That's right. I have updated that section as you suggested. > > Best, > David > > On Tue, May 24, 2022 at 5:39 PM José Armando García Sancio > wrote: > > > > Hi David, > > > > Thanks for the KIP. In the "Compatibility, Deprecation, and Migration > > Plan", you have: > > > > > The change is backward compatible. > > > > I assume that we don't need to increase the metadata.version/IBP for > > AlterPartition because AlterPartitionManager uses ApiVersions for that > > channel. Should we mention that in that section? > > > > -- > > -José >
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi all, Thanks for your feedback. I just updated the KIP as follows: * I propose to add a `TopicId` field at the same time while we are bumping the AlterPartition API version. * I propose to add the `InShuttingDown` field to the registration records to track if a broker is in controlled shutdown. This is useful for debugging and it would also prevent unnecessary AlterPartition requests from getting sent to the controller by the leaders. This will also help for KAFKA-13944. Artem - That's a good question. As explained by Colin we don't have this notion of fenced replicas in the ZK world. José - That's right. I have updated that section as you suggested. Best, David On Tue, May 24, 2022 at 5:39 PM José Armando García Sancio wrote: > > Hi David, > > Thanks for the KIP. In the "Compatibility, Deprecation, and Migration > Plan", you have: > > > The change is backward compatible. > > I assume that we don't need to increase the metadata.version/IBP for > AlterPartition because AlterPartitionManager uses ApiVersions for that > channel. Should we mention that in that section? > > -- > -José
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi David, Thanks for the KIP. In the "Compatibility, Deprecation, and Migration Plan", you have: > The change is backward compatible. I assume that we don't need to increase the metadata.version/IBP for AlterPartition because AlterPartitionManager uses ApiVersions for that channel. Should we mention that in that section? -- -José
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi David, Thanks for the KIP. LGTM! best, Colin On Wed, May 18, 2022, at 08:55, David Jacot wrote: > Hi, > > I created a small KIP to strengthen the AlterPartition API in KRaft mode: > https://cwiki.apache.org/confluence/x/phmhD > > Let me know what you think. > > Best, > David
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi Artem, In ZK mode, the concept of fenced replicas does not exist -- brokers are either in the cluster or out, and if they're out, we scrub all information about them from the metadata. (With the exception of replica sets they are in.) best, Colin On Fri, May 20, 2022, at 10:17, Artem Livshits wrote: > The KIP LGTM. My only question is why it's an issue with KRaft -- looks > like ZK would have the same issue? > > -Artem > > On Fri, May 20, 2022 at 8:51 AM David Jacot > wrote: > >> This KIP is pretty straight forward. I will start a vote on Monday >> if no one objects. >> >> Best, >> David >> >> On Wed, May 18, 2022 at 5:55 PM David Jacot wrote: >> > >> > Hi, >> > >> > I created a small KIP to strengthen the AlterPartition API in KRaft mode: >> > https://cwiki.apache.org/confluence/x/phmhD >> > >> > Let me know what you think. >> > >> > Best, >> > David >>
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
The KIP LGTM. My only question is why it's an issue with KRaft -- looks like ZK would have the same issue? -Artem On Fri, May 20, 2022 at 8:51 AM David Jacot wrote: > This KIP is pretty straight forward. I will start a vote on Monday > if no one objects. > > Best, > David > > On Wed, May 18, 2022 at 5:55 PM David Jacot wrote: > > > > Hi, > > > > I created a small KIP to strengthen the AlterPartition API in KRaft mode: > > https://cwiki.apache.org/confluence/x/phmhD > > > > Let me know what you think. > > > > Best, > > David >
Re: [DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
This KIP is pretty straight forward. I will start a vote on Monday if no one objects. Best, David On Wed, May 18, 2022 at 5:55 PM David Jacot wrote: > > Hi, > > I created a small KIP to strengthen the AlterPartition API in KRaft mode: > https://cwiki.apache.org/confluence/x/phmhD > > Let me know what you think. > > Best, > David
[DISCUSS] KIP-841: Fenced replicas should not be allowed to join the ISR in KRaft
Hi, I created a small KIP to strengthen the AlterPartition API in KRaft mode: https://cwiki.apache.org/confluence/x/phmhD Let me know what you think. Best, David
