On Wed, Jan 26, 2022, at 09:14, José Armando García Sancio wrote: > Thanks for the feedback Colin. > > Colin wrote: >> 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. > > Sounds good. I am going to call it LeaderRecoveryState. > > Colin wrote: >> 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 discussed this offline. We agree that it is better to be explicit > with regards to changes to the leader recovery state and not make the > changes implicit. The controller is going to have checks that make > sure that the transitions are valid. For example, the controller will > not allow the topic partition leader to: > 1. Change the ISR to a size greater than 1 if the leader is still recovering. > 2. Change the leader recovery state from "recovered" to "recovering". > > We also agree that we will add the field in the response just to be > consistent with the existing pattern. We should create another KIP to > remove these fields as they are not strictly necessary. >
+1. Thanks, José! cheers, Colin
