squah-confluent commented on code in PR #23888:
URL: https://github.com/apache/kafka/pull/23888#discussion_r4336524242
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/share/ShareGroup.java:
##########
@@ -97,6 +99,33 @@ public record InitMapValue(
Set<Integer> partitions,
long timestamp
) {
+ public InitMapValue {
+ partitions = Set.copyOf(partitions);
+ }
+
+ public InitMapValue(String name, Collection<Integer> partitions, long
timestamp) {
+ this(name, Set.copyOf(partitions), timestamp);
+ }
+
+ /**
+ * @param added The partitions to add.
+ * @return A copy of this value with the partitions added.
+ */
+ public InitMapValue withAddedPartitions(Collection<Integer> added) {
+ Set<Integer> newPartitions = new HashSet<>(partitions);
+ newPartitions.addAll(added);
+ return new InitMapValue(name, newPartitions, timestamp);
Review Comment:
I prefer `copyOf` in the constructor since it's truly immutable and it's not
possible to misuse by holding on to the original collection and modifying it.
But it does end up with more copies overall.
I think our original convention was to use `Collections.unmodifiableMap/Set`
in constructors, so I'll go with that. Though the streams code is pulling a
different way and going with `copyOf` lately...
--
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]
