Thank you for the reviews. Vikas, > > In the case of an already-reassigning partition being reassigned again, the validation compares the targetReplicaSet size of the reassignment to the targetReplicaSet size of the new reassignment and throws if those differ. > Can you add more detail to this, or clarify what is targetReplicaSet (for e.g. why not sourceReplicaSet?) and how the target replica set will be calculated? If a reassignment is ongoing, such that [1,2,3] => [4,5,6] (the replica set in Kafka will be [1,2,3,4,5,6] during the reassignment), and you try to issue a new reassignment (e.g [7,8,9], Kafka should NOT think that the RF of the partition is 6 just because a reassignment is ongoing. Hence, we compare [4,5,6]'s length to [7,8,9] The targetReplicaSet is a term we use in KIP-455 <https://cwiki.apache.org/confluence/display/KAFKA/KIP-455%3A+Create+an+Administrative+API+for+Replica+Reassignment>. It means the desired replica set that a given reassignment is trying to achieve. Here we compare said set of the on-going reassignment to the new reassignment.
Note this can happen during an RF change too. e.g [1,2,3] => [4,5,6,7] (RF change, intermediate set is [1,2,3,4,5,6,7]), and we try to do a reassignment to [9,10,11], the logic will compare [4,5,6,7] to [9,10,11]. In such a situation where one wants to cancel the RF increase and reassign again, one first needs to cancel the existing reassignment via the API (no special action required despite RF change) > And what about the reassign partitions CLI? Do we want to expose the option there too? Yes, this is already present in the KIP if I'm not mistaken. We describe it in "Accordingly, the kafka-reassign-partitions.sh tool will be updated to allow supplying the new option:" I have edited the KIP to contain two clear paragraphs called Admin API and CLI now. Colin, > it would be nice for the first paragraph to be a bit more explicit about this goal. sounds good, updated it with that suggestion. > client-side forward compatibility I was under the assumption that it is not recommended to upgrade clients before brokers, but a quick search cleared it up to me that we're pretty intentional about allowing that <https://www.confluent.io/blog/upgrading-apache-kafka-clients-just-got-easier/> . Do you happen to know if we have any policy on client-side forward compatibility with regard to such things -- extending "write" APIs (that mutate the state) with fields that conditionally limit that modification? It seems like a rare use case to me, hence renaming it to something like tryDisableReplicationFactorChange may unnecessary impair the API. Would Admin API documentation that says "this is supported only from brokers on version X and above. If not supported, the default behavior (no replication factor guards) is applied" be sufficient? Best, Stanislav On Fri, Aug 5, 2022 at 8:32 PM Colin McCabe <cmcc...@apache.org> wrote: > Hi Stanislav, > > Thanks for the KIP. I think this is a nice solution to the problem of not > wanting to change the replication factor during reassignments. > > Just from a writing point of view, it would be nice for the first > paragraph to be a bit more explicit about this goal. Maybe lead with "Many > times, we don't want to change the replication factor of a partition during > reassignment..." As it is, we're talking about metadata races before we've > even explained what the goal is that the metadata races are thwarting. :) > > I like the RPC and command-line format changes; they are well-done and > well-written. One thing we do need to spell out, those, is what the > behavior is when the server does not support this new option. The simplest > thing to do would be for the client to throw UnsupportedVersionException > with an exception message indicating what the problem is. Then the caller > could catch this and re-try the call without the flag (or give up, as > appropriate?) > > The other option is to continue on but not actually protect replication > factor. If we do this, at minimum we'd need to rename the flag something > like "try to protect replication factor" to make it clear that it's > best-effort. > > It's sort of debatable which way is better. In principle the UVE sounds > nicer, but in practice maybe the other behavior is best? I suspect most > systems would turn around and retry without the flag in the event of a > UVE... > > best, > Colin > > > On Thu, Aug 4, 2022, at 13:37, Vikas Singh wrote: > > Thanks Stanislav for the KIP. Seems like a reasonable proposal, > > preventing users from accidentally altering the replica set under certain > > conditions. I have couple of comments: > > > > > >> In the case of an already-reassigning partition being reassigned again, > > the validation compares the targetReplicaSet size of the reassignment to > > the targetReplicaSet size of the new reassignment and throws if those > > differ. > > Can you add more detail to this, or clarify what is targetReplicaSet (for > > e.g. why not sourceReplicaSet?) and how the target replica set will be > > calculated? > > > > And what about the reassign partitions CLI? Do we want to expose the > option > > there too? > > > > Cheers, > > Vikas > > > > On Thu, Jul 28, 2022 at 1:59 AM Stanislav Kozlovski < > stanis...@confluent.io> > > wrote: > > > >> Hey all, > >> > >> I'd like to start a discussion on a proposal to help API users from > >> inadvertently increasing the replication factor of a topic through > >> the alter partition reassignments API. The KIP describes two fairly > >> easy-to-hit race conditions in which this can happen. > >> > >> The KIP itself is pretty simple, yet has a couple of alternatives that > can > >> help solve the same problem. I would appreciate thoughts from the > community > >> on how you think we should proceed, and whether the proposal makes > sense in > >> the first place. > >> > >> Thanks! > >> > >> KIP: > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments > >> JIRA: https://issues.apache.org/jira/browse/KAFKA-14121 > >> > >> -- > >> Best, > >> Stanislav > >> > -- Best, Stanislav
