apoorvmittal10 commented on code in PR #18864: URL: https://github.com/apache/kafka/pull/18864#discussion_r1957427989
########## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/SimpleAssignor.java: ########## @@ -72,36 +77,178 @@ private GroupAssignment assignHomogenous( if (subscribeTopicIds.isEmpty()) return new GroupAssignment(Collections.emptyMap()); - Map<Uuid, Set<Integer>> targetPartitions = computeTargetPartitions( + // Subscribed topic partitions for the share group. + List<TargetPartition> targetPartitions = computeTargetPartitions( subscribeTopicIds, subscribedTopicDescriber); - return new GroupAssignment(groupSpec.memberIds().stream().collect(Collectors.toMap( - Function.identity(), memberId -> new MemberAssignmentImpl(targetPartitions)))); + // The current assignment from topic partition to members. + Map<TargetPartition, List<String>> currentAssignment = currentAssignment(groupSpec); + return newAssignmentHomogeneous(groupSpec, subscribeTopicIds, targetPartitions, currentAssignment); } private GroupAssignment assignHeterogeneous( GroupSpec groupSpec, SubscribedTopicDescriber subscribedTopicDescriber ) { - Map<String, MemberAssignment> members = new HashMap<>(); + Map<String, List<TargetPartition>> memberToPartitionsSubscription = new HashMap<>(); for (String memberId : groupSpec.memberIds()) { MemberSubscription spec = groupSpec.memberSubscription(memberId); if (spec.subscribedTopicIds().isEmpty()) continue; - Map<Uuid, Set<Integer>> targetPartitions = computeTargetPartitions( + // Subscribed topic partitions for the share group member. + List<TargetPartition> targetPartitions = computeTargetPartitions( spec.subscribedTopicIds(), subscribedTopicDescriber); + memberToPartitionsSubscription.put(memberId, targetPartitions); + } + + // The current assignment from topic partition to members. + Map<TargetPartition, List<String>> currentAssignment = currentAssignment(groupSpec); + return newAssignmentHeterogeneous(groupSpec, memberToPartitionsSubscription, currentAssignment); + } - members.put(memberId, new MemberAssignmentImpl(targetPartitions)); + // Get the current assignment for subscribed topic partitions to share group members. + private Map<TargetPartition, List<String>> currentAssignment(GroupSpec groupSpec) { + Map<TargetPartition, List<String>> assignment = new HashMap<>(); + + for (String member : groupSpec.memberIds()) { + Map<Uuid, Set<Integer>> assignedTopicPartitions = groupSpec.memberAssignment(member).partitions(); + assignedTopicPartitions.forEach((topicId, partitions) -> partitions.forEach( + partition -> assignment.computeIfAbsent(new TargetPartition(topicId, partition), k -> new ArrayList<>()).add(member))); } + return assignment; + } + + private GroupAssignment newAssignmentHomogeneous( + GroupSpec groupSpec, + Set<Uuid> subscribeTopicIds, + List<TargetPartition> targetPartitions, + Map<TargetPartition, List<String>> currentAssignment) { + + Map<TargetPartition, List<String>> newAssignment = new HashMap<>(); + // Step 1: Hash member IDs to partitions. + memberHashAssignment(targetPartitions, groupSpec.memberIds(), newAssignment); + + // Step 2: Round-robin assignment for unassigned partitions which do not have members already assigned in the current assignment. + Set<TargetPartition> assignedPartitions = new HashSet<>(newAssignment.keySet()); + List<TargetPartition> unassignedPartitions = targetPartitions.stream() + .filter(targetPartition -> !assignedPartitions.contains(targetPartition)) + .filter(targetPartition -> !currentAssignment.containsKey(targetPartition)) + .collect(Collectors.toList()); + + roundRobinAssignment(groupSpec.memberIds(), unassignedPartitions, newAssignment); + + Map<String, Set<TargetPartition>> finalAssignment = new HashMap<>(); + + // When combining current assignment, we need to only consider the topics in current assignment that are also being + // subscribed in the new assignment as well. + currentAssignment.forEach((targetPartition, members) -> { + if (subscribeTopicIds.contains(targetPartition.topicId)) + members.forEach(member -> { + if (groupSpec.memberIds().contains(member)) + finalAssignment.computeIfAbsent(member, k -> new HashSet<>()).add(targetPartition); + }); + }); + newAssignment.forEach((targetPartition, members) -> members.forEach(member -> + finalAssignment.computeIfAbsent(member, k -> new HashSet<>()).add(targetPartition))); + + return groupAssignment(finalAssignment, groupSpec.memberIds()); + } + + private GroupAssignment newAssignmentHeterogeneous( + GroupSpec groupSpec, + Map<String, List<TargetPartition>> memberToPartitionsSubscription, + Map<TargetPartition, List<String>> currentAssignment) { + + // Exhaustive set of all subscribed topic partitions. + Set<TargetPartition> targetPartitions = new LinkedHashSet<>(); + memberToPartitionsSubscription.values().forEach(targetPartitions::addAll); + + // Create a map for topic to members subscription. + Map<Uuid, Set<String>> topicToMemberSubscription = new HashMap<>(); + memberToPartitionsSubscription.forEach((member, partitions) -> + partitions.forEach(partition -> topicToMemberSubscription.computeIfAbsent(partition.topicId(), k -> new LinkedHashSet<>()).add(member))); + + Map<TargetPartition, List<String>> newAssignment = new HashMap<>(); + // Step 1: Hash member IDs to partitions. + memberToPartitionsSubscription.forEach((member, partitions) -> + memberHashAssignment(partitions, Collections.singletonList(member), newAssignment)); + + // Step 2: Round-robin assignment for unassigned partitions which do not have members already assigned in the current assignment. + Set<TargetPartition> assignedPartitions = new LinkedHashSet<>(newAssignment.keySet()); + Map<Uuid, List<TargetPartition>> unassignedPartitions = new HashMap<>(); + targetPartitions.forEach(targetPartition -> { + if (!assignedPartitions.contains(targetPartition) && !currentAssignment.containsKey(targetPartition)) + unassignedPartitions.computeIfAbsent(targetPartition.topicId(), k -> new ArrayList<>()).add(targetPartition); + }); + + unassignedPartitions.keySet().forEach(unassignedTopic -> + roundRobinAssignment(topicToMemberSubscription.get(unassignedTopic), unassignedPartitions.get(unassignedTopic), newAssignment)); + + // Step 3: We combine current assignment and new assignment. + Map<String, Set<TargetPartition>> finalAssignment = new HashMap<>(); + + // When combining current assignment, we need to only consider the member topic subscription in current assignment + // which is being subscribed in the new assignment as well. + currentAssignment.forEach((targetPartition, members) -> members.forEach(member -> { + if (topicToMemberSubscription.getOrDefault(targetPartition.topicId(), Collections.emptySet()).contains(member)) + finalAssignment.computeIfAbsent(member, k -> new HashSet<>()).add(targetPartition); + })); + newAssignment.forEach((targetPartition, members) -> members.forEach(member -> + finalAssignment.computeIfAbsent(member, k -> new HashSet<>()).add(targetPartition))); + + return groupAssignment(finalAssignment, groupSpec.memberIds()); + } + + private GroupAssignment groupAssignment( + Map<String, Set<TargetPartition>> assignmentByMember, + Collection<String> allGroupMembers) { + Map<String, MemberAssignment> members = new HashMap<>(); + for (Map.Entry<String, Set<TargetPartition>> entry : assignmentByMember.entrySet()) { + Map<Uuid, Set<Integer>> targetPartitions = new HashMap<>(); + entry.getValue().forEach(targetPartition -> targetPartitions.computeIfAbsent(targetPartition.topicId(), k -> new HashSet<>()).add(targetPartition.partition())); + members.put(entry.getKey(), new MemberAssignmentImpl(targetPartitions)); + } + allGroupMembers.forEach(member -> { + if (!members.containsKey(member)) + members.put(member, new MemberAssignmentImpl(new HashMap<>())); + }); + return new GroupAssignment(members); } - private Map<Uuid, Set<Integer>> computeTargetPartitions( + // Visible for testing. + void memberHashAssignment( + List<TargetPartition> targetPartitions, + Collection<String> memberIds, + Map<TargetPartition, List<String>> assignment) { + if (!targetPartitions.isEmpty()) + for (String memberId : memberIds) { + int topicPartitionIndex = Math.abs(memberId.hashCode() % targetPartitions.size()); + TargetPartition topicPartition = targetPartitions.get(topicPartitionIndex); + assignment.computeIfAbsent(topicPartition, k -> new ArrayList<>()).add(memberId); + } + } + + // Visible for testing. + void roundRobinAssignment( + Collection<String> members, + List<TargetPartition> partitions, + Map<TargetPartition, List<String>> assignment) { + Iterator<String> memberIterator = members.iterator(); + for (TargetPartition targetPartition : partitions) { + if (!memberIterator.hasNext()) { + memberIterator = members.iterator(); + } + String member = memberIterator.next(); Review Comment: What we are trying to do here, can you please write comments. -- 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