jackylee-ch commented on code in PR #12351:
URL: https://github.com/apache/gluten/pull/12351#discussion_r3479877723
##########
gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java:
##########
@@ -199,44 +199,56 @@ public boolean ringContain(long slot) {
}
private boolean add(T node) {
- boolean added = false;
- if (node != null && !nodes.containsKey(node)) {
- Set<Partition<T>> partitions =
- IntStream.range(0, replicate)
- .mapToObj(idx -> new Partition<T>(node, idx))
- .collect(Collectors.toSet());
- nodes.put(node, partitions);
-
- // allocate slot.
- for (Partition<T> partition : partitions) {
- long slot;
- int seed = 0;
- do {
- slot = this.hasher.hash(partition.getPartitionKey(), seed++);
- } while (ring.containsKey(slot));
-
- partition.setSlot(slot);
- ring.put(slot, partition);
- }
- added = true;
+ if (node == null) {
+ return false;
+ }
+ // Validate the key before any map lookup: nodes.containsKey() invokes the
node's
+ // hashCode()/equals(), which may themselves dereference key() and NPE on
a null key, masking
+ // the intended fail-fast here.
+ Preconditions.checkArgument(node.key() != null, "Node key must not be
null: %s", node);
+ if (nodes.containsKey(node)) {
+ return false;
}
- return added;
+ Set<Partition<T>> partitions =
+ IntStream.range(0, replicate)
+ .mapToObj(idx -> new Partition<T>(node, idx))
+ .collect(Collectors.toSet());
+ nodes.put(node, partitions);
+
+ // allocate slot.
+ for (Partition<T> partition : partitions) {
+ long slot;
+ int seed = 0;
+ do {
+ slot = this.hasher.hash(partition.getPartitionKey(), seed++);
+ } while (ring.containsKey(slot));
+
+ partition.setSlot(slot);
+ ring.put(slot, partition);
+ }
+ return true;
}
public static class Partition<T extends ConsistentHash.Node> {
private final T node;
private final int index;
+ // Hash the node by its logical key() so the ring is stable and
reproducible across JVMs (avoid
+ // node.toString(), which may fall back to the identity hash). Computed
once: the key is
+ // immutable and re-read on every collision-retry in add().
+ private final String partitionKey;
+
private long slot;
public Partition(T node, int index) {
this.node = node;
this.index = index;
+ this.partitionKey = node.key() + ":" + index;
}
public String getPartitionKey() {
- return String.format("%s:%d", node, index);
+ return partitionKey;
Review Comment:
We actually implemented `ExecutorNode.toString` so that its value matches
`key()`. However, the current implementation looks better.
--
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]
