Thanks for the feedback Colin and Luke. Colin wrote: > The KIP talks a bit about "recovery," which is a new term (as far as I > know). If I understand correctly, this is a state that the partition enters > into after an unclean leader election. I would suggest using a different > term for this, since we already use the term "log recovery" to express the > cleanup we do after an unclean broker restart. How about something like > "leader cleansing"? Whatever term we pick, we should probably use it in all > the metadata rather than is_unclean, etc.
How about "IsLeaderRecovering"? Colin wrote: > I think we also need to spell out a few other things about "cleansing" > state. For example, if the controller needs to change the leader of the > partition again, before cleansing completes, does that mean the partition > is still unclean? I would guess yes. We should spell this out in the KIP. Currently, when an unclean leader election is performed the leader is set to an online replica and the ISR is set to the elected leader. This KIP is proposing that the topic partition leader will not allow follower replicas to join the ISR while it is recovering. This means that the ISR cannot become greater than 1 until the current topic partition leader has recovered. This also means that if a new leader is elected while the partition is still recovering then the new leader will also be because of an unclean leader election. Colin wrote: > With regard to AlterIsr, it seems like we might end up in a scenario where > the broker is on an old version that doesn't support the cleansing state, > and the controller is on a new version that does. In that case, it seems > like we could get stuck in the cleansing state for an arbitrary amount of > time. We will not exit it until the broker sends an AlterIsr, but the > broker may not send one of those ever (for example, if we go down to one > uncleanly elected replica, and stay there). This is correct but I think this is okay and doesn't require an IBP bump for the following reasons: 1. If the leader doesn't support this feature and it never sends an AlterIsr request, it means that the ISR never changed so the leader cannot change. 2. If while "is recovering" is true the broker of the leader is upgraded to a version that supports this feature, then this leader will get re-elected again and the "is recovering" flag will be true. This upgraded leader will now recover and eventually send an AlterIsr request with the "is recovering" flag set to false. Colin wrote: > I think the compatibility logic needs needs more thought. One example is, > if we have some brokers in the cluster that support KIP-704 and some that > don't, do we auto-clear the cleansing state when transitioning leadership > uncleanly to an old broker? I think you probably have to. So we should > discuss this in the KIP. I'll update the KIP to talk about these cases and explain how they will be handled. Luke wrote: > In the `PartitionChangeRecord` schema, you set the `isUnclean` field as > type boolean, > but the value (-1, 0, 1) are all int values. Did you mean int32 type or the > value should be updated? Good catch. Updated the type for that field to int8. Thanks! -José
