Hey Jose, Thanks for the updates. I noticed that `LeaderRecoveryState` is marked as ignorable in the `AlterPartition` request. It would be helpful to understand the motivation for that.
Thanks, Jason On Wed, Jan 26, 2022 at 10:25 AM Colin McCabe <cmcc...@apache.org> wrote: > 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 >
