Copilot commented on code in PR #12379:
URL: https://github.com/apache/gluten/pull/12379#discussion_r3479719029
##########
gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java:
##########
@@ -162,7 +162,10 @@ public Set<T> getNodes() {
public Set<Partition<T>> getPartition(T node) {
lock.readLock().lock();
try {
- return nodes.get(node);
+ // Return a defensive copy: the map value is internal mutable state, and
getNodes() copies
+ // for the same reason. Callers get a snapshot they can't use to mutate
the ring.
+ Set<Partition<T>> partitions = nodes.get(node);
+ return partitions == null ? null : new HashSet<>(partitions);
Review Comment:
Returning a new HashSet prevents callers from mutating the *set*, but the
elements are still the same internal mutable Partition instances. Since
Partition exposes a public setSlot(...), external callers can still corrupt the
ring (e.g., change a partition slot so removeNode later removes the wrong
entries). If getPartition is intended to be a safe snapshot accessor, consider
making Partition immutable (or at least make setSlot non-public) so callers
can’t mutate internal state through the returned 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
