dajac commented on code in PR #23742:
URL: https://github.com/apache/kafka/pull/23742#discussion_r4215916943
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/Assignment.java:
##########
@@ -42,9 +42,14 @@ public class Assignment implements MemberAssignment {
public Assignment(
Map<Uuid, Set<Integer>> partitions
) {
+ Objects.requireNonNull(partitions);
// Assignments are used as input to assignors, which expect to receive
immutable
// assignment maps, otherwise they will be modified in place.
- this.partitions =
Collections.unmodifiableMap(Objects.requireNonNull(partitions));
+ Map<Uuid, Set<Integer>> copy = new HashMap<>(partitions.size());
+ partitions.forEach(
+ (topicId, partitionsIds) -> copy.put(topicId,
Set.copyOf(partitionsIds))
+ );
Review Comment:
We need to review how we use Assignment because we may have undesired copies
with this change. In general, I wonder if should make the constructor private
and have helper with explicit copy or not. e.g. Assignment.copyOf,
Assignment.of, etc. Would it help?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/Assignment.java:
##########
@@ -85,7 +90,7 @@ public static Assignment fromRecord(
return new Assignment(
record.topicPartitions().stream().collect(Collectors.toMap(
ConsumerGroupTargetAssignmentMemberValue.TopicPartition::topicId,
- topicPartitions -> new
HashSet<>(topicPartitions.partitions())))
+ topicPartitions -> Set.copyOf(topicPartitions.partitions())))
Review Comment:
We copy here and then re-copy in the Assignment constructor. We should try
to void it.
--
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]
