Merge pull request #43 from uber/faultybug

Do not assume update type for first time member

index.js

@@ -161,7 +161,8 @@ RingPop.prototype.setupChannel = function setupChannel() {
 RingPop.prototype.addLocalMember = function addLocalMember(info) {
     this.membership.addMember({
         address: this.hostPort,
-        incarnationNumber: info && info.incarnationNumber
+        incarnationNumber: info && info.incarnationNumber,
+        status: 'alive'
     });
 };
 
@@ -531,23 +532,6 @@ RingPop.prototype.onMemberFaulty = function onMemberFaulty(member) {
     this.suspicion.stop(member);
 };
 
-RingPop.prototype.onMemberJoined = function onMemberJoined(member) {
-    this.stat('increment', 'membership-update.new');
-    this.logger.debug('member has joined', {
-        local: this.membership.localMember.address,
-        joined: member.address
-    });
-
-    this.ring.addServer(member.address);
-
-    this.dissemination.addChange({
-        address: member.address,
-        status: member.status,
-        incarnationNumber: member.incarnationNumber,
-        piggybackCount: 0
-    });
-};
-
 RingPop.prototype.onMemberLeave = function onMemberLeave(member) {
     this.stat('increment', 'membership-update.leave');
     this.logger.debug('member has left', {
@@ -589,19 +573,16 @@ RingPop.prototype.onMembershipUpdated = function onMembershipUpdated(updates) {
     var ringChanged = false;
 
     updates.forEach(function(update) {
-        if (update.type === 'alive') {
+        if (update.status === 'alive') {
             self.onMemberAlive(update);
             ringChanged = membershipChanged = true;
-        } else if (update.type === 'faulty') {
+        } else if (update.status === 'faulty') {
             self.onMemberFaulty(update);
             ringChanged = membershipChanged = true;
-        } else if (update.type === 'leave') {
+        } else if (update.status === 'leave') {
             self.onMemberLeave(update);
             ringChanged = membershipChanged = true;
-        } else if (update.type === 'new') {
-            self.onMemberJoined(update);
-            ringChanged = membershipChanged = true;
-        } else if (update.type === 'suspect') {
+        } else if (update.status === 'suspect') {
             self.onMemberSuspect(update);
             membershipChanged = true;
         }

lib/members.js

@@ -153,23 +153,23 @@ Membership.evalOverride = function evalOverride(member, change) {
         // it affirms its "aliveness" and bumps its incarnation number.
         member.status = 'alive';
         member.incarnationNumber = +new Date();
-        return _.extend({ type: 'alive' }, member);
+        return member;
     } else if (Membership.isAliveOverride(member, change)) {
         member.status = 'alive';
         member.incarnationNumber = change.incarnationNumber || member.incarnationNumber;
-        return _.extend({ type: 'alive' }, member);
+        return member;
     } else if (Membership.isSuspectOverride(member, change)) {
         member.status = 'suspect';
         member.incarnationNumber = change.incarnationNumber || member.incarnationNumber;
-        return _.extend({ type: 'suspect' }, member);
+        return member;
     } else if (Membership.isFaultyOverride(member, change)) {
         member.status = 'faulty';
         member.incarnationNumber = change.incarnationNumber || member.incarnationNumber;
-        return _.extend({ type: 'faulty' }, member);
+        return member;
     } else if (Membership.isLeaveOverride(member, change)) {
         member.status = 'leave';
         member.incarnationNumber = change.incarnationNumber || member.incarnationNumber;
-        return _.extend({ type: 'leave' }, member);
+        return member;
     }
 };
 
@@ -217,12 +217,12 @@ Membership.isPingable = function isPingable(member) {
 
 Membership.prototype.addMember = function addMember(member, force, noEvent) {
     if (!force && this.hasMember(member)) {
-        return;
+        return member;
     }
 
     var newMember = {
         address: member.address,
-        status: member.status || 'alive',
+        status: member.status,
         incarnationNumber: member.incarnationNumber || +new Date(),
         isLocal: this.ringpop.hostPort === member.address
     };
@@ -234,8 +234,10 @@ Membership.prototype.addMember = function addMember(member, force, noEvent) {
     this.members.splice(this.getJoinPosition(), 0, newMember);
 
     if (!noEvent) {
-        this._emitUpdated(_.extend({ type: 'new' }, newMember));
+        this._emitUpdated(newMember);
     }
+
+    return newMember;
 };
 
 Membership.prototype.affirmAliveness = function affirmAliveness() {
@@ -426,22 +428,26 @@ Membership.prototype.update = function update(changes) {
 
     for (var i = 0 ; i < changes.length; i++) {
         var change = changes[i];
-        var member = this.findMemberByAddress(change.address);
 
-        if (member) {
-            var update = Membership.evalOverride(member, change);
+        var member = this.findMemberByAddress(change.address);
 
-            if (update) {
-                updates.push(update);
-            }
-        } else {
-            member = {
+        if (!member) {
+            // This is the first time ringpop is hearing about
+            // this member. No need to evaluate override rules.
+            member = this.addMember({
                 address: change.address,
                 status: change.status,
                 incarnationNumber: change.incarnationNumber
-            };
-            this.addMember(member, true, true);
-            updates.push(_.extend(member, { type: 'new' }));
+            }, true, true);
+
+            updates.push(member);
+            continue;
+        }
+
+        var override = Membership.evalOverride(member, change);
+
+        if (override) {
+            updates.push(override);
         }
     }
 

lib/ring.js

@@ -36,24 +36,40 @@ util.inherits(HashRing, EventEmitter);
 // TODO - error checking around adding a server that's already there
 // TODO - error checking from rbtree.insert
 HashRing.prototype.addServer = function addServer(name) {
+    if (this.hasServer(name)) {
+        return;
+    }
+
     this.servers[name] = true;
+
     for (var i = 0; i < this.replicaPoints; i++) {
         this.rbtree.insert(farmhash.hash32(name + i), name);
     }
+
     this.emit('added', name);
 };
 
 HashRing.prototype.getServerCount = function getServerCount() {
     return Object.keys(this.servers).length;
 };
 
+HashRing.prototype.hasServer = function hasServer(name) {
+    return !!this.servers[name];
+};
+
 // TODO - error checking around removing servers that aren't there
 // TODO - error checking from rbtree.insert
 HashRing.prototype.removeServer = function removeServer(name) {
+    if (!this.hasServer(name)) {
+        return;
+    }
+
     delete this.servers[name];
+
     for (var i = 0; i < this.replicaPoints; i++) {
         this.rbtree.remove(farmhash.hash32(name + i), name);
     }
+
     this.emit('removed', name);
 };
 

test/index_test.js

@@ -139,7 +139,10 @@ test('admin leave stops suspicion subprotocol', function t(assert) {
 
     var ringpop = createRingpop();
     ringpop.addLocalMember({ incarnationNumber: 1 });
-    ringpop.membership.addMember(ringpopRemote.membership.localMember);
+    ringpop.membership.addMember({
+        address: ringpopRemote.membership.localMember.address,
+        status: 'alive'
+    });
     ringpop.suspicion.start(ringpopRemote.hostPort);
 
     ringpop.adminLeave(function(err) {
@@ -310,7 +313,10 @@ test('emits membership changed event', function t(assert) {
 
     var ringpop = createRingpop();
     ringpop.addLocalMember();
-    ringpop.membership.addMember({ address: node1Addr });
+    ringpop.membership.addMember({
+        address: node1Addr,
+        status: 'alive'
+    });
 
     function assertChanged() {
         ringpop.once('membershipChanged', function onMembershipChanged() {
@@ -337,7 +343,10 @@ test('emits ring changed event', function t(assert) {
 
     var ringpop = createRingpop();
     ringpop.addLocalMember();
-    ringpop.membership.addMember({ address: node1Addr });
+    ringpop.membership.addMember({
+        address: node1Addr,
+        status: 'alive'
+    });
 
     function assertChanged(changer) {
         ringpop.once('membershipChanged', function onMembershipChanged() {
@@ -370,3 +379,21 @@ test('emits ring changed event', function t(assert) {
     ringpop.destroy();
     assert.end();
 });
+
+test('first time member, not alive', function t(assert) {
+    var ringpop = createRingpop();
+    ringpop.addLocalMember();
+
+    var faultyAddr = '127.0.0.1:3001';
+    ringpop.membership.update([{
+        address: faultyAddr,
+        status: 'faulty',
+        incarnationNumber: Date.now()
+    }]);
+
+    assert.notok(ringpop.ring.hasServer(faultyAddr),
+        'new faulty server should not be in ring');
+
+    ringpop.destroy();
+    assert.end();
+});

test/members_test.js

@@ -40,7 +40,7 @@ test('change with higher incarnation number results in leave override', function
 
     var update = Membership.evalOverride(member, change);
 
-    assert.equals(update.type, 'leave', 'results in leave');
+    assert.equals(update.status, 'leave', 'results in leave');
     assert.end();
 });