Copilot commented on code in PR #20000:
URL: https://github.com/apache/kafka/pull/20000#discussion_r2598156989
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/assignor/AssignorHelpers.java:
##########
@@ -69,4 +71,34 @@ static <K, V> HashMap<K, V> newHashMap(int numMappings) {
static <K> HashSet<K> newHashSet(int numElements) {
return new HashSet<>((int) (((numElements + 1) / 0.75f) + 1));
}
+
+
+ /**
+ * Checks if the member's rack matches any of the partition's racks.
+ * @param memberRackId The member's rack id.
+ * @param partitionRackIds The partition's rack ids.
+ * @return True if the member's rack matches any of the partition's racks,
false otherwise.
+ */
+ public static boolean isRackMatch(Optional<String> memberRackId,
Set<String> partitionRackIds) {
+ return memberRackId.isPresent() &&
partitionRackIds.contains(memberRackId.get());
+ }
+
+ /**
+ * Determines whether rack-aware assignment should be used based on the
provided racks.
+ * @param allMemberRacks The set of all member racks.
+ * @param allPartitionRacks The set of all partition racks.
+ * @param racksPerPartition A map of partitions to their respective racks.
+ * @return True if member racks and partition racks overlap and not all
partitions have the same set of racks, false otherwise.
+ */
+ public static boolean useRackAwareAssignment(
+ Set<String> allMemberRacks,
+ Set<String> allPartitionRacks,
+ Map<TopicIdPartition, Set<String>> racksPerPartition
+ ) {
+ if (allMemberRacks.isEmpty() || Collections.disjoint(allMemberRacks,
allPartitionRacks))
+ return false;
+ else {
+ return
!racksPerPartition.values().stream().allMatch(allPartitionRacks::equals);
+ }
Review Comment:
[nitpick] The if-else statement could be simplified by removing the else
block and directly returning the condition. Consider:
```java
return !allMemberRacks.isEmpty()
&& !Collections.disjoint(allMemberRacks, allPartitionRacks)
&&
!racksPerPartition.values().stream().allMatch(allPartitionRacks::equals);
```
This is more concise and avoids the unnecessary else block.
```suggestion
return !allMemberRacks.isEmpty()
&& !Collections.disjoint(allMemberRacks, allPartitionRacks)
&&
!racksPerPartition.values().stream().allMatch(allPartitionRacks::equals);
```
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/assignor/CommonAssignorTests.java:
##########
@@ -123,9 +124,14 @@ public static void testAssignmentReuse(PartitionAssignor
assignor, SubscriptionT
);
for (String memberId : members.keySet()) {
- // The assignment map from the assignor must be the same as the
immutable assignment map
- // that went in.
- assertSame(membersWithAssignment.get(memberId).partitions(),
secondAssignment.members().get(memberId).partitions());
+ if (rackAware) {
+ // With rack awareness, the assignment maps may be mutable
cause of revoking non-matched partitions.
Review Comment:
Grammatical error: "cause of" should be "because of".
```suggestion
// With rack awareness, the assignment maps may be mutable
because of revoking non-matched partitions.
```
--
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]
