[GLUTEN-12378][CORE] Return a defensive copy from ConsistentHash.getPartition()#12379
[GLUTEN-12378][CORE] Return a defensive copy from ConsistentHash.getPartition()#12379LuciferYang wants to merge 2 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
This PR strengthens ConsistentHash’s encapsulation/thread-safety contract by ensuring getPartition() does not expose the internal mutable Set stored in the nodes map, aligning it with the existing defensive-copy behavior of other accessors (e.g., getNodes()).
Changes:
- Update
ConsistentHash.getPartition()to return a defensive copy of the partition set (ornullif the node is absent). - Add a unit test that verifies mutating the returned set does not affect the ring’s internal state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java | Returns a defensive copy from getPartition() instead of exposing the internal set. |
| gluten-core/src/test/java/org/apache/gluten/hash/ConsistentHashTest.java | Adds a regression test ensuring callers can’t mutate the ring by clearing the returned partition set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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); |
There was a problem hiding this comment.
Good point — addressed in 4d10483. setSlot() is now private (only add() assigns slots during construction, accessible as a nestmate), so callers can no longer mutate ring state through the partitions returned by getPartition(). I kept it to the minimal non-public change rather than restructuring construction for full Partition immutability, since that would touch add()/getPartitionKey() — which #12351 is already changing — and I'd rather not overlap the two PRs.
|
Run Gluten Clickhouse CI on x86 |
…artition() getPartition() returned the internal partition set directly, so a caller could mutate the ring's state through it and the reference escaped after the read lock was released. Every other accessor (e.g. getNodes()) already copies. Return a defensive copy for consistency and to keep the @threadsafe contract intact. Add a test that mutating the returned set does not affect the ring.
…n() is a safe snapshot The defensive copy stops callers from mutating the returned set, but the Partition elements were still shared and setSlot() was public, so a caller could change a slot and corrupt the ring (e.g. removeNode() would then drop the wrong entry). Only add() assigns slots during construction, so make setSlot private (accessible as a nestmate).
LuciferYang
force-pushed
the
gluten-12378-getpartition-defensive-copy
branch
from
4d10483 to
12f31b9
Compare
|
Run Gluten Clickhouse CI on x86 |
What changes are proposed in this pull request?
ConsistentHashis@ThreadSafeand its accessors return defensive copies — exceptgetPartition(), which returned the internal set directly:That set is the same instance held in the internal
nodesmap, so a caller could mutate the ring through it (e.g.getPartition(node).clear()), and the reference escaped after the read lock was released. This PR makesgetPartition()a safe snapshot accessor:getNodes().Partition.setSlot()non-public. Copying the set alone still left thePartitionelements shared and mutable; sincesetSlot()was public, a caller could change a slot and corrupt the ring (e.g. soremoveNode()later drops the wrong entry). Onlyadd()assigns slots during construction, sosetSlot()is nowprivate(reached as a nestmate).This is latent today —
getPartition()has no production caller and the internal set isn't mutated after a node is added — so there's no user-facing change; it's an encapsulation/consistency fix.Fixes #12378.
How was this patch tested?
Added a unit test that adds a node, clears the set returned by
getPartition(), and verifies the ring still reports all partitions. MakingsetSlot()non-public is enforced at compile time (no external caller exists). The existingConsistentHashTestcases still pass.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)