For a little background on why `AlterIsr` returns the state back in the response, originally the idea was that the partition leader could use the response to reset its own state after a failed request. The tricky thing for ISR changes is always ensuring that the partition leader reflects a worst-case assumption about the current ISR to avoid advancing the high watermark incorrectly. Ultimately I do not think we used error responses as intended, so we might not have a hard requirement to keep all of these fields. I'd probably suggest sticking with the current approach and revisiting in a separate KIP, but I don't feel strongly about it.
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`. 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. Both of these suggestions make sense to me. Best, Jason On Fri, Jan 21, 2022 at 11:07 AM José Armando García Sancio <jsan...@confluent.io.invalid> 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. > > Thanks > -José >
