philipnee commented on code in PR #13550:
URL: https://github.com/apache/kafka/pull/13550#discussion_r1179185376


##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractStickyAssignor.java:
##########
@@ -149,47 +148,57 @@ private boolean allSubscriptionsEqual(Set<String> 
allTopics,
             }
 
             MemberData memberData = memberData(subscription);
+            final int memberGeneration = 
memberData.generation.orElse(DEFAULT_GENERATION);
+            maxGeneration = Math.max(maxGeneration, memberGeneration);
 
             List<TopicPartition> ownedPartitions = new ArrayList<>();
             consumerToOwnedPartitions.put(consumer, ownedPartitions);
 
-            // Only consider this consumer's owned partitions as valid if it 
is a member of the current highest
-            // generation, or it's generation is not present but we have not 
seen any known generation so far
-            if (memberData.generation.isPresent() && 
memberData.generation.get() >= maxGeneration
-                || !memberData.generation.isPresent() && maxGeneration == 
DEFAULT_GENERATION) {
-
-                // If the current member's generation is higher, all the 
previously owned partitions are invalid
-                if (memberData.generation.isPresent() && 
memberData.generation.get() > maxGeneration) {
-                    allPreviousPartitionsToOwner.clear();
-                    partitionsWithMultiplePreviousOwners.clear();
-                    for (String droppedOutConsumer : 
membersOfCurrentHighestGeneration) {
-                        
consumerToOwnedPartitions.get(droppedOutConsumer).clear();
-                    }
-
-                    membersOfCurrentHighestGeneration.clear();
-                    maxGeneration = memberData.generation.get();
-                }
+            // the member has a valid generation, so we can consider its owned 
partitions if it has the highest
+            // generation amongst
+            for (final TopicPartition tp : memberData.partitions) {
+                if (allTopics.contains(tp.topic())) {
+                    String otherConsumer = 
allPreviousPartitionsToOwner.put(tp, consumer);
+                    if (otherConsumer == null) {
+                        // this partition is not owned by other consumer in 
the same generation
+                        ownedPartitions.add(tp);
+                    } else {
+                        final int otherMemberGeneration = 
subscriptions.get(otherConsumer).generationId().orElse(DEFAULT_GENERATION);
 
-                membersOfCurrentHighestGeneration.add(consumer);
-                for (final TopicPartition tp : memberData.partitions) {
-                    // filter out any topics that no longer exist or aren't 
part of the current subscription
-                    if (allTopics.contains(tp.topic())) {
-                        String otherConsumer = 
allPreviousPartitionsToOwner.put(tp, consumer);
-                        if (otherConsumer == null) {
-                            // this partition is not owned by other consumer 
in the same generation
-                            ownedPartitions.add(tp);
-                        } else {
+                        if (memberGeneration == otherMemberGeneration) {
                             log.error("Found multiple consumers {} and {} 
claiming the same TopicPartition {} in the "
-                                + "same generation {}, this will be 
invalidated and removed from their previous assignment.",
-                                     consumer, otherConsumer, tp, 
maxGeneration);
-                            
consumerToOwnedPartitions.get(otherConsumer).remove(tp);
+                                            + "same generation {}, this will 
be invalidated and removed from their previous assignment.",
+                                    consumer, otherConsumer, tp, 
memberGeneration);
                             partitionsWithMultiplePreviousOwners.add(tp);
+                            
consumerToOwnedPartitions.get(otherConsumer).remove(tp);
+                            allPreviousPartitionsToOwner.put(tp, consumer);
+                        } else if (memberGeneration > otherMemberGeneration) {
+                            // move partition from the member with an older 
generation to the member with the newer generation
+                            consumerToOwnedPartitions.get(consumer).add(tp);
+                            
consumerToOwnedPartitions.get(otherConsumer).remove(tp);
+                            allPreviousPartitionsToOwner.put(tp, consumer);
+                            // if memberGeneration > otherMemberGeneration, 
the other member continue owns the generation
+                            log.warn("{} in generation {} and {} in generation 
{} claiming the same TopicPartition {} in " +
+                                            "different generations. The topic 
partition wil be assigned to the member with " +
+                                            "the higher generation {}.",
+                                    consumer, memberGeneration,
+                                    otherConsumer, otherMemberGeneration,
+                                    tp,
+                                    memberGeneration);
+                        } else {
+                            // if memberGeneration < otherMemberGeneration, 
the other member continue owns the generation

Review Comment:
   thanks, there's also a typo: continue to own the "topic partition"



-- 
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

Reply via email to