Skip to content

[GLUTEN-12378][CORE] Return a defensive copy from ConsistentHash.getPartition() #12379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
LuciferYang wants to merge 2 commits into
base: main
Choose a base branch
from
+23 −2
Draft

[GLUTEN-12378][CORE] Return a defensive copy from ConsistentHash.getPartition() #12379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
674cfe7
[GLUTEN-12378][CORE] Return a defensive copy from ConsistentHash.getP…
LuciferYang Jun 26, 2026
12f31b9
[GLUTEN-12378][CORE] Make Partition.setSlot non-public so getPartitio…
LuciferYang Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +165 to +168

@LuciferYang LuciferYang Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} finally {
lock.readLock().unlock();
}
Expand Down Expand Up @@ -255,7 +258,11 @@ public T getNode() {
return node;
}

public void setSlot(long slot) {
// Non-public on purpose: only ConsistentHash.add() assigns the slot, during ring construction
// (accessible as a nestmate). Keeping it out of the public API stops callers of getPartition()
// from mutating ring state through a returned Partition (e.g. changing a slot so removeNode()
// drops the wrong entry).
private void setSlot(long slot) {
this.slot = slot;
}

Expand Down
14 changes: 14 additions & 0 deletions gluten-core/src/test/java/org/apache/gluten/hash/ConsistentHashTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,20 @@ public String key() {
});
}

@Test
public void testGetPartitionReturnsDefensiveCopy() {
HostNode node = new HostNode("executor-1");
consistentHash.addNode(node);

Set<ConsistentHash.Partition<ConsistentHash.Node>> partitions =
consistentHash.getPartition(node);
Assert.assertEquals(REPLICAS, partitions.size());

// Mutating the returned set must not affect the ring's internal state.
partitions.clear();
Assert.assertEquals(REPLICAS, consistentHash.getPartition(node).size());
}

private static class HostNode implements ConsistentHash.Node {
private final String host;

Expand Down
Loading