From d7c1d4d1e5df63d1372eca9537d57dd2cf6bdfb0 Mon Sep 17 00:00:00 2001 From: Jeff Wolski Date: Thu, 5 Mar 2015 13:21:22 -0800 Subject: [PATCH] Do not assume update type for first time member --- index.js | 31 ++++++------------------------- lib/members.js | 44 +++++++++++++++++++++++++------------------- lib/ring.js | 16 ++++++++++++++++ test/index_test.js | 33 ++++++++++++++++++++++++++++++--- test/members_test.js | 2 +- 5 files changed, 78 insertions(+), 48 deletions(-) diff --git a/index.js b/index.js index 223e444e..a9348013 100644 --- a/index.js +++ b/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; } diff --git a/lib/members.js b/lib/members.js index 887f9558..61073643 100644 --- a/lib/members.js +++ b/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); } } diff --git a/lib/ring.js b/lib/ring.js index 12cf5712..9677f7b1 100644 --- a/lib/ring.js +++ b/lib/ring.js @@ -36,10 +36,16 @@ 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); }; @@ -47,13 +53,23 @@ 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); }; diff --git a/test/index_test.js b/test/index_test.js index d07c6659..7a534fc4 100644 --- a/test/index_test.js +++ b/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(); +}); diff --git a/test/members_test.js b/test/members_test.js index 65fe5ad6..c97971fc 100644 --- a/test/members_test.js +++ b/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(); });