showuon commented on a change in pull request #10985:
URL: https://github.com/apache/kafka/pull/10985#discussion_r668514017
##########
File path:
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractStickyAssignor.java
##########
@@ -205,6 +237,9 @@ private boolean allSubscriptionsEqual(Set<String> allTopics,
// consumer owned the "maxQuota" of partitions or more, and
we're still under the number of expected members
// with more than the minQuota partitions, so keep "maxQuota"
of the owned partitions, and revoke the rest of the partitions
numMembersAssignedOverMinQuota++;
+ if (numMembersAssignedOverMinQuota ==
expectedNumMembersAssignedOverMinQuota) {
+ potentiallyUnfilledMembersAtMinQuota.clear();
Review comment:
I tried to summarize both methods:
What @guozhangwang 's meaning is, we can "lazily" detect the issue after
assigning all unassignedPartitions. we don't need to clear the
`potentiallyUnfilledMembersAtMinQuota` here, because as the "original" variable
naming said: they are "potentially unfilled members", just keep them there. We
"should not" assign partitions to them in this case because we've reached
`expectedNumMembersAssignedOverMinQuota`.
But if somehow, after assigning unassignedPartitions to all unfilledMembers,
there are still some unassignedPartitions left. We can just assign them to
`potentiallyUnfilledMembersAtMinQuota`. And after running out the
`unassignedPartitions`, we can check:
```
numMembersWithMaxQuota == expectedNumMembersWithMaxQuota &&
numMembersWithMinQuota == expectedNumMembersWithMinQuota
```
to do error handling.
VS.
In @ableegoldman 's version , we find issue immediately and handle it. we
computed the `potentiallyUnfilledMembersAtMinQuota` correctly (that's why we
need to clear it). So, if the issue happened:
> if somehow, after assigning unassignedPartitions to all unfilledMembers,
there are still some unassignedPartitions left
We can try to get member from `potentiallyUnfilledMembersAtMinQuota` and
then assign unassignedPartition to the member. If we can't get member from it
(i.e. `potentiallyUnfilledMembersAtMinQuota` is empty), we throw exception
directly.
Both ways can find errors when happened. Personally, I like Sophie's version
more since it's much clear.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
