lucliu1108 commented on PR #24062: URL: https://github.com/apache/kafka/pull/24062#issuecomment-6549588492
Hi @chia7712, this PR now has two commits, and they correspond to two different approaches. I'd like your opinion on which one to adapt. ### Option 1: always set adding and removing (same method suggested in the ticket) This only removes the two `isEmpty()` checks, so the new adding and removing sets are always passed to `PartitionChangeBuilder`. It fixes the three examples in the description, but it regresses this case compared with trunk: 1. Start with replicas=[0,1], ISR=[0,1]. 2. Reassign to [2,3]. The partition becomes replicas=[2,3,0,1], adding=[2,3], removing=[0,1], and 2 and 3 start copying data. 3. Before 2 and 3 catch up, resubmit the same target [2,3]. This happens, for example, when running `kafka-reassign-partitions.sh --execute --additional` again with the same JSON to change the throttle. The new delta against [2,3,0,1] is adding=[] and removing=[0,1], so the partition becomes replicas=[2,3,0,1], adding=[], removing=[0,1]. 4. Cancel the reassignment. Cancel removes the replicas currently in adding, and since adding is now [], it removes nothing, so the partition ends up with replicas=[2,3,0,1] instead of going back to [0,1]. On trunk, step 3 writes no record, adding stays [2,3], and the cancel reverts to [0,1]. If the reassignment is not cancelled, it still completes correctly with [2,3] once 2 and 3 join the ISR. Until then, the broker computes the replication factor as replicas minus adding, which is 4 instead of 2, so the partition is reported as under-replicated, and `listPartitionReassignments` reports adding=[] for replicas that are still catching up. The same happens for any new target that keeps previous adding replicas without adding new ones. For example, narrowing [2,3] to [2,0] also gives adding=[]. ### Option 2: keep previous adding replicas and only wait for the ones that are not being removed (current version) It makes 2 more changes compared to option 1 1. The new adding set also includes the previous adding replicas, so replicas minus adding is always the original replicas and cancel always reverts correctly. 2. When deciding whether the reassignment can complete, it only waits for adding replicas that are not also being removed (`pendingAdding`, which is adding minus removing). Without this, example 1 in the description would still wait for 2 and 3 to join the ISR. With Option 2, resubmitting [2,3] is a no-op again, and cancelling after resubmitting or after narrowing reverts to [0,1]. The trade-off is that a replica can now be in both adding and removing. For example, [0,1] -> [2,3] -> [2,0] gives replicas=[2,0,1,3], adding=[2,3], removing=[1,3]. This is visible through `listPartitionReassignments`, and it contradicts the `PartitionReassignment` javadoc, which describes `addingReplicas` as "the brokers that we are adding this partition to". External tools that assume the two lists are disjoint could misreport such a reassignment. Option 2 also changes the outcome of two examples in the description compared with Option 1. In example 2, the reassignment waits for both 3 and 4 to join the ISR, since 3 is still a new replica that never caught up. In example 3, it waits for 2 and 3 instead of completing immediately. Both still end with the requested assignment. --- Seems that option1 has a niche case, my question is: is it acceptable to change the meaning of `addingReplicas` as in Option 2, or would you prefer to go back to Option 1 and accept the cancel regression? I'm not sure if my understanding of those expected behavior (especially what cancel should revert to after a reassignment has been placed), so please correct me if I got anything wrong here, thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
