On Fri, Jan 21, 2022, at 11:07, José Armando García Sancio wrote: > Hi all, > > The following suggestions are not strictly required to implement this > KIP but what do we think about: > > 1. Changing the name of the AlterIsr RPC to AlterPartition RPC. > > 2. Change the name of the field "CurrentIsrVersion" to > "PartitionEpoch". This is the name that we use in the KRaft > implementation and the PartitionRecord.
+1 for both. On Mon, Jan 24, 2022, at 14:59, José Armando García Sancio wrote: > Okay. The current KIP only supports two election recovery states > because the type used is a boolean. I am going to extend it to use an > int8 and support the following states: > > 1. ELECTION_RECOVERY - Notify to the leader that it should recover the > local log. It indicates to the followers that the leader is > recovering. > 2. ELECTION_RECOVERED - Notify to the leader that recovery is not > needed. It indicates to the follower that the leader has recovered. We already have many classes that are called "partition state." For example, PartitionStates.java on the client side, PartitionStateMachine.scala and TopicPartitionStateZNode in the old controller, RemotePartitionDeleteState.java under storage, and so forth. I don't object to adding another one, but let's make it very clear that it's LeaderRecoveryState not just a generic "partition state", to avoid confusion. Actually maybe we should call it LeaderRecoveryStateChange, since we'll need to have a "no change" entry in the enum. On Fri, Jan 21, 2022, at 11:02, José Armando García Sancio wrote: > I am mainly returning the recorded value for consistency with the > existing fields in the AlterIsrResponse. The implementation on the > broker (AlterIsrManager and Partition) takes advantage of this by > updating it's local state using the response values if the > "CurrentIsrVersion" validates. I would argue that we should not add unused fields to RPCs and metadata records because we might want them in the future, or because it seems more "symmetrical," etc. We have a great mechanism for adding new stuff in the future: add a new field and specify the default as whatever the old behavior was. So I would argue that we should not add this state to AlterIsr, either the request or the response. We already know that sending AlterIsr clears the recovery state, and if it succeeded then the state was cleared. If this changes in the future, we can add a new field that default to whatever we want. Adding an RPC field that will only ever have one value is bad form. And 99% of the time, when you do finally decide to have more than one possible value, you'll find that what you originally wrote isn't adequate and you need to change the RPC, or the code, anyway. At least, that's my experience. We should follow the YAGNI philosophy for metadata record fields and RPC fields. If you don't have a use case for something yet, wait until you do have a use case for it to add it. best, Colin On Mon, Jan 24, 2022, at 14:59, José Armando García Sancio wrote: > Thanks for the additional context regarding AlterIsrResponse. > > Jason wrote: >> In regard to the naming of `IsLeaderRecovering`, I agree it still seems a >> bit awkward. I kind of liked the idea of turning it into a `PartitionState` >> field instead. That would also address the inconsistent type in the >> `PartitionChangeRecord`. > > Okay. The current KIP only supports two election recovery states > because the type used is a boolean. I am going to extend it to use an > int8 and support the following states: > > 1. ELECTION_RECOVERY - Notify to the leader that it should recover the > local log. It indicates to the followers that the leader is > recovering. > 2. ELECTION_RECOVERED - Notify to the leader that recovery is not > needed. It indicates to the follower that the leader has recovered. > > In the current KIP is the partition state invariant is > "!InElectionRecovery || size(ISR) == 1". With the above change the > invariant will be "ElectionState == ELECTION_RECOVERED || size(ISR) == > 1". > > Thanks! > -José
