Repository navigation
MINOR: Make share group state partition metadata unmodifiable - #23888
squah-confluent merged 5 commits into
Conversation
InitMapValue now copies its partitions with Set.copyOf, so that the share group state partition metadata can be shared without defensive copies, such as with assignors offloaded to another thread. Add withAddedPartitions and withRemovedPartitions to derive new values, and a constructor taking a Collection for the partition lists in records. Use them in GroupMetadataManager and remove the copies that are no longer needed. combineInitMaps reuses a value when only one of the maps has the topic, and initializeShareGroupState no longer deep copies the initializing topics to remove partitions in place. initializedShareGroupPartitions and sharePartitionsEligibleForOffsetDeletion no longer copy the partitions. The partitions of the immutable sets are iterated in an unspecified order, so tests now compare them regardless of order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| public record ShareGroupStatePartitionMetadataInfo( | ||
| Map<Uuid, InitMapValue> initializingTopics, | ||
| Map<Uuid, InitMapValue> initializedTopics, | ||
| Set<Uuid> deletingTopics | ||
| ) { | ||
| } |
There was a problem hiding this comment.
While we are here, should we ensure that the two Maps and the Set are immutable too?
There was a problem hiding this comment.
Yes, we can do that too
| Set<Integer> newPartitions = new HashSet<>(partitions); | ||
| newPartitions.addAll(added); | ||
| return new InitMapValue(name, newPartitions, timestamp); |
There was a problem hiding this comment.
It is a tad wasteful to allocate an HashSet here to then copy it in InitMapValue. Have you considered using unmodifiableSet?
There was a problem hiding this comment.
Do you mean replacing Set.copyOf with Collections.unmodifiableSet in the constructor or passing an unmodifiableSet wrapper in to the existing constructor?
There was a problem hiding this comment.
Either way is fine. I don't really have a preference and I don't think that we have established a pattern (yet).
There was a problem hiding this 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...
There was a problem hiding this comment.
The current version of the PR uses unmodifiableMap/Set.
| Set<Integer> newPartitions = new HashSet<>(partitions); | ||
| newPartitions.removeAll(removed); | ||
| return new InitMapValue(name, newPartitions, timestamp); |
c461d8c to
088c095
Compare
squah-confluent
changed the title
…of copying them Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fiable Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
088c095 to
1189796
Compare
| * @param added The partitions to add. | ||
| * @return A copy of this value with the partitions added. | ||
| */ | ||
| public InitMapValue withAddedPartitions(Collection<Integer> added) { |
There was a problem hiding this comment.
We always use it with Set. I wonder if we should directly use it here too.
There was a problem hiding this comment.
Do we need to unit tests those new methods?
There was a problem hiding this comment.
We always use it with
Set. I wonder if we should directly use it here too.
Yes, we can take Sets.
Do we need to unit tests those new methods?
Yes, those do need some basic tests. Added unit tests
| * @param removed The partitions to remove. | ||
| * @return A copy of this value with the partitions removed. | ||
| */ | ||
| public InitMapValue withRemovedPartitions(Collection<Integer> removed) { |
…edPartitions Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Wrap InitMapValue's partitions and
ShareGroupStatePartitionMetadataInfo's collections in unmodifiable
views, so that the share group state partition metadata cannot be
modified through them. Add withAddedPartitions and withRemovedPartitions
to InitMapValue to derive new values.
Use them in GroupMetadataManager and remove the copies that are no
longer needed. combineInitMaps reuses a value when only one of the maps
has the topic, and initializeShareGroupState no longer deep copies the
initializing topics to remove partitions in place.
initializedShareGroupPartitions and
sharePartitionsEligibleForOffsetDeletion no longer copy the partitions.
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Reviewers: David Jacot dajac@apache.org