[Fix] Consistent hash ring lookups 1 / Use comparator function (#315)

* Rename RingNode.val to key
* Rename RingNode.str to value
* Extract rbtree comparison into function

lib/ring/index.js

@@ -30,10 +30,13 @@ function HashRing(options) {
     this.replicaPoints = this.options.replicaPoints || 100;
     this.hashFunc = this.options.hashFunc || farmhash.hash32v1;
 
-    this.rbtree = new RBTree();
+    this.rbtree = new RBTree(HashRing.comparator);
     this.servers = {};
     this.checksum = null;
 }
+HashRing.comparator = function comparator(a, b) {
+    return a - b;
+};
 
 util.inherits(HashRing, EventEmitter);
 
@@ -151,10 +154,10 @@ HashRing.prototype.removeServerReplicas = function removeServerReplicas(server)
 HashRing.prototype.lookup = function lookup(str) {
     var hash = this.hashFunc(str);
     var iter = this.rbtree.upperBound(hash);
-    var res = iter.str();
+    var res = iter.value();
     if (res === null) {
         var min = this.rbtree.min();
-        res = min && min.str;
+        res = min && min.value;
     }
     return res;
 };
@@ -175,9 +178,9 @@ HashRing.prototype.lookupN = function lookupN(str, n) {
     // (This is to guard against the cases where serverCount is out of sync with
     // the rbtree (e.g., due to a bug), e.g., n === serverCount === 3 but the
     // rbtree contains only 2 unique servers.)
-    var firstVal = iter.val();
+    var firstVal = iter.key();
     do {
-        var res = iter.str();
+        var res = iter.value();
         if (res === null) {
             // reached end of rbtree, wrapping around
             iter = this.rbtree.iterator();
@@ -189,7 +192,7 @@ HashRing.prototype.lookupN = function lookupN(str, n) {
             }
         }
         iter.next();
-    } while (resultArray.length < n && iter.val() !== firstVal);
+    } while (resultArray.length < n && iter.key() !== firstVal);
 
     return resultArray;
 };

lib/ring/rbtree.js

@@ -20,10 +20,10 @@
 
 var RBIterator; // forward references
 
-// payload of this tree (val, str) is embedded into the node for performance
-function RingNode(val, str) {
-    this.val = val;
-    this.str = str;
+// payload of this tree (key, value) is embedded into the node for performance
+function RingNode(key, value) {
+    this.key = key;
+    this.value = value;
     this.left = null;
     this.right = null;
     this.red = true;
@@ -33,15 +33,16 @@ RingNode.prototype.getChild = function(dir) {
     return dir ? this.right : this.left;
 };
 
-RingNode.prototype.setChild = function(dir, val) {
+RingNode.prototype.setChild = function(dir, child) {
     if (dir) {
-        this.right = val;
+        this.right = child;
     } else {
-        this.left = val;
+        this.left = child;
     }
 };
 
-var RBTree = function RBTree() {
+var RBTree = function RBTree(comparator) {
+    this.comparator = comparator;
     this.root = null;
     this.size = 0;
 };
@@ -68,11 +69,11 @@ function doubleRotate(root, dir) {
 }
 
 // returns true if inserted, false if duplicate
-RBTree.prototype.insert = function(val, str) {
+RBTree.prototype.insert = function(key, value) {
     var ret = false;
 
     if (this.root === null) { // empty
-        this.root = new RingNode(val, str);
+        this.root = new RingNode(key, value);
         ret = true;
     } else {
         var head = new RingNode(undefined, undefined); // fake tree root
@@ -89,7 +90,7 @@ RBTree.prototype.insert = function(val, str) {
         while (true) {
             if (node === null) {
                 // insert new node at the bottom
-                node = new RingNode(val, str);
+                node = new RingNode(key, value);
                 p.setChild(dir, node);
                 ret = true;
             } else if (isRed(node.left) && isRed(node.right)) {
@@ -110,7 +111,7 @@ RBTree.prototype.insert = function(val, str) {
                 }
             }
 
-            var cmp = node.val - val; // note inlined comparitor
+            var cmp = this.comparator(node.key, key);
 
             // stop if found
             if (cmp === 0) {
@@ -150,7 +151,7 @@ RBTree.prototype.insert = function(val, str) {
 // delete a red node.
 
 // returns true if found and removed, false if not found
-RBTree.prototype.remove = function(val) {
+RBTree.prototype.remove = function(key) {
     if (this.root === null) {
         return false;
     }
@@ -171,7 +172,7 @@ RBTree.prototype.remove = function(val) {
         p = node;
         node = node.getChild(dir);
 
-        var cmp = val - node.val; // note inlined comparitor
+        var cmp = this.comparator(key, node.key);
 
         dir = cmp > 0;
 
@@ -217,8 +218,8 @@ RBTree.prototype.remove = function(val) {
 
     // splice out the node that we've found
     if (found !== null) {
-        found.val = node.val;
-        found.str = node.str;
+        found.key = node.key;
+        found.value = node.value;
         p.setChild(p.right === node, node.getChild(node.left === null));
         this.size--;
     }
@@ -233,12 +234,12 @@ RBTree.prototype.remove = function(val) {
 };
 
 // Returns an interator to the tree node at or immediately after the item
-RBTree.prototype.lowerBound = function(val) {
+RBTree.prototype.lowerBound = function(key) {
     var cur = this.root;
     var iter = this.iterator();
 
     while (cur !== null) {
-        var c = val - cur.val;
+        var c = this.comparator(key, cur.key);
         if (c === 0) {
             iter.cursor = cur;
             return iter;
@@ -249,7 +250,7 @@ RBTree.prototype.lowerBound = function(val) {
 
     for (var i = iter.ancestors.length - 1; i >= 0; --i) {
         cur = iter.ancestors[i];
-        if (val - cur.val < 0) {
+        if (this.comparator(key, cur.key) < 0) {
             iter.cursor = cur;
             iter.ancestors.length = i;
             return iter;
@@ -261,10 +262,10 @@ RBTree.prototype.lowerBound = function(val) {
 };
 
 // Returns an interator to the tree node immediately after the item
-RBTree.prototype.upperBound = function(val) {
-    var iter = this.lowerBound(val);
+RBTree.prototype.upperBound = function(key) {
+    var iter = this.lowerBound(key);
 
-    while (iter.val() !== null && (iter.val() - val) < 0) { // inlined comparitor
+    while (iter.key() !== null && this.comparator(iter.key(), key) < 0) {
         iter.next();
     }
 
@@ -295,12 +296,12 @@ RBIterator = function RBIterator(tree) {
     this.cursor = null;
 };
 
-RBIterator.prototype.val = function() {
-    return this.cursor !== null ? this.cursor.val : null;
+RBIterator.prototype.key = function() {
+    return this.cursor !== null ? this.cursor.key : null;
 };
 
-RBIterator.prototype.str = function() {
-    return this.cursor !== null ? this.cursor.str : null;
+RBIterator.prototype.value = function() {
+    return this.cursor !== null ? this.cursor.value : null;
 };
 
 // if null-iterator, return first node, otherwise next node
@@ -330,7 +331,7 @@ RBIterator.prototype.next = function() {
             this.minNode(this.cursor.right);
         }
     }
-    return this.cursor !== null ? this.cursor.val : null;
+    return this.cursor !== null ? this.cursor.key : null;
 };
 
 // find the left-most node from this subtree

test/unit/rbiterator_test.js

@@ -20,10 +20,12 @@
 
 var RBTree = require('../../lib/ring/rbtree').RBTree;
 var RBIterator = require('../../lib/ring/rbtree').RBIterator;
+var comparator = require('../../lib/ring').comparator;
+
 var test = require('tape');
 
 test('construct a new RBIterator', function t(assert) {
-    var tree = new RBTree();
+    var tree = new RBTree(comparator);
     var iterator = new RBIterator(tree);
 
     assert.strictEquals(iterator.tree, tree, 'tree is set to supplied tree');
@@ -32,26 +34,26 @@ test('construct a new RBIterator', function t(assert) {
     assert.end();
 });
 
-test('RBIterator.val and RBIterator.str', function t(assert) {
-    var tree = new RBTree();
+test('RBIterator.key and RBIterator.value', function t(assert) {
+    var tree = new RBTree(comparator);
     var iterator = new RBIterator(tree);
 
-    assert.strictEquals(iterator.val(), null, 'val on empty tree is null');
-    assert.strictEquals(iterator.str(), null, 'str on empty tree is null');
+    assert.strictEquals(iterator.key(), null, 'key on empty tree is null');
+    assert.strictEquals(iterator.value(), null, 'value on empty tree is null');
 
     iterator.cursor = {
-        val: 1234,
-        str: '1234'
+        key: 1234,
+        value: '1234'
     };
 
-    assert.strictEquals(iterator.val(), 1234, 'val returns cursor val');
-    assert.strictEquals(iterator.str(), '1234', 'str returns cursor str');
+    assert.strictEquals(iterator.key(), 1234, 'key returns cursor key');
+    assert.strictEquals(iterator.value(), '1234', 'value returns cursor value');
 
     assert.end();
 });
 
 function makeTree() {
-    var tree = new RBTree();
+    var tree = new RBTree(comparator);
 
     tree.insert(1, 'one');
     tree.insert(2, 'two');
@@ -78,16 +80,16 @@ test('RBIterator.minNode', function t(assert) {
     var iterator = new RBIterator(tree);
 
     iterator.minNode(tree.root);
-    assert.strictEquals(iterator.val(), 1, 'val min from root is 1');
-    assert.strictEquals(iterator.str(), 'one', 'str min from root is one');
+    assert.strictEquals(iterator.key(), 1, 'key min from root is 1');
+    assert.strictEquals(iterator.value(), 'one', 'value min from root is one');
 
     iterator.minNode(tree.root.left.left);
-    assert.strictEquals(iterator.val(), 1, 'val min from 1 is 1');
-    assert.strictEquals(iterator.str(), 'one', 'str min from 1 is one');
+    assert.strictEquals(iterator.key(), 1, 'key min from 1 is 1');
+    assert.strictEquals(iterator.value(), 'one', 'value min from 1 is one');
 
     iterator.minNode(tree.root.right);
-    assert.strictEquals(iterator.val(), 5, 'val min from 6 is 5');
-    assert.strictEquals(iterator.str(), 'five', 'str min from 6 is five');
+    assert.strictEquals(iterator.key(), 5, 'key min from 6 is 5');
+    assert.strictEquals(iterator.value(), 'five', 'value min from 6 is five');
 
     assert.end();
 });
@@ -96,15 +98,15 @@ test('RBIterator.next walk the entire tree', function t(assert) {
     var tree = makeTree();
     var iterator = new RBIterator(tree);
 
-    assert.strictEquals(iterator.next(), 1, 'val is 1');
-    assert.strictEquals(iterator.next(), 2, 'val is 2');
-    assert.strictEquals(iterator.next(), 3, 'val is 3');
-    assert.strictEquals(iterator.next(), 4, 'val is 4');
-    assert.strictEquals(iterator.next(), 5, 'val is 5');
-    assert.strictEquals(iterator.next(), 6, 'val is 6');
-    assert.strictEquals(iterator.next(), 7, 'val is 7');
-    assert.strictEquals(iterator.next(), 8, 'val is 8');
-    assert.strictEquals(iterator.next(), null, 'val is null');
+    assert.strictEquals(iterator.next(), 1, 'key is 1');
+    assert.strictEquals(iterator.next(), 2, 'key is 2');
+    assert.strictEquals(iterator.next(), 3, 'key is 3');
+    assert.strictEquals(iterator.next(), 4, 'key is 4');
+    assert.strictEquals(iterator.next(), 5, 'key is 5');
+    assert.strictEquals(iterator.next(), 6, 'key is 6');
+    assert.strictEquals(iterator.next(), 7, 'key is 7');
+    assert.strictEquals(iterator.next(), 8, 'key is 8');
+    assert.strictEquals(iterator.next(), null, 'key is null');
 
     assert.end();
 });