vamossagar12 commented on code in PR #12561:
URL: https://github.com/apache/kafka/pull/12561#discussion_r975980445
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorIncrementalTest.java:
##########
@@ -517,13 +517,13 @@ public void testTaskAssignmentWhenWorkerBounces() {
leaderAssignment = deserializeAssignment(result, leaderId);
assertAssignment(leaderId, offset,
Collections.emptyList(), 0,
- Collections.emptyList(), 0,
+ Collections.emptyList(), 1,
Review Comment:
TBH, even I didn't want to re-introduce the flag back but seemed the easiest
way to get around the regression. I guess, as you said it might be easier to
work through other issues on the allocation algorithm to finally have the flag
redundant.
Regarding the side-effects of the re-introduction of this flag, I had
imagined that adding the flag back would break some of the tests but that
didn't happen which may or mayn't be a good thing. I did look at the logic
again and compared with the original algorithm it seemed to me that this line:
https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignor.java#L310
is the line that prevented successive revoking rebalances. The other check
here:
https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignor.java#L312
gives us a window to set it to true and force a revocation in the next round
which kind of made me believe that it should be a safe check. That said, if
there are scenarios where we think we need testing, I would be happy to do that.
--
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]
