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. Thanks -José
