LuciferYang commented on code in PR #12351:
URL: https://github.com/apache/gluten/pull/12351#discussion_r3465183566
##########
gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java:
##########
@@ -201,6 +201,7 @@ public boolean ringContain(long slot) {
private boolean add(T node) {
boolean added = false;
if (node != null && !nodes.containsKey(node)) {
+ Preconditions.checkArgument(node.key() != null, "Node key must not be
null: %s", node);
Set<Partition<T>> partitions =
Review Comment:
Good catch — fixed in fc574dd. The null-key check now runs before
`nodes.containsKey(node)`, so a Node whose hashCode()/equals() reads key()
fails fast with the IllegalArgumentException instead of NPEing inside the
lookup.
##########
gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java:
##########
@@ -236,7 +237,10 @@ public Partition(T node, int index) {
}
public String getPartitionKey() {
- return String.format("%s:%d", node, index);
+ // Hash the node by its logical key() so the ring is stable and
reproducible across JVMs.
+ // Avoid node.toString() (which may default to the identity hash) and
String.format (slow on
+ // this hot path, called per virtual node during add()).
+ return node.key() + ":" + index;
Review Comment:
Done in fc574dd — the partition key is now computed once in the Partition
constructor and stored in a final field, so key() isn't re-invoked on each
collision-retry in add().
--
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]
