chickenchickenlove commented on code in PR #23742:
URL: https://github.com/apache/kafka/pull/23742#discussion_r4246736860
##########
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:
@dajac
Thanks for your comments!
It helps me a lot. 🙇♂️
You are right. I've created new commit to address it.
I've made the constructor private and introduced `Assignment.of()` and
`Assignment.copyOf()` to make the copying behavior explicit. I've also removed
the redundant copying in `fromRecord()`.
When processing the `assignor`'s output, we check whether it returned the
same map instance that was provided as input. If so, we use `Assignment.of()`
to reuse the existing immutable data. Otherwise, we use `Assignment.copyOf()`
because the returned map and its partition sets may be mutable.
What do you think?
--
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]
