Skip to content

Commit

Permalink
Simplify override logic during updates (#260)
Browse files Browse the repository at this point in the history 
Simplify the logic inside override-checks to be on par with ringpop-go and make it easier to read the code.
  • Loading branch information
Menno Pruijssers committed May 11, 2016
1 parent af0574b commit 656d620
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 62 deletions.
88 changes: 36 additions & 52 deletions lib/membership/member.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,68 +171,28 @@ Member.prototype._applyUpdatePenalty = function _applyUpdatePenalty() {
};

Member.prototype._isLocalOverride = function _isLocalOverride(update) {
var self = this;

return isLocalFaultyOverride() || isLocalSuspectOverride() || isLocalTombstoneOverride();

function isLocalFaultyOverride() {
return self.ringpop.whoami() === self.address &&
update.status === Member.Status.faulty;
if (this.ringpop.whoami() !== this.address) {
return false;
}

function isLocalSuspectOverride() {
return self.ringpop.whoami() === self.address &&
update.status === Member.Status.suspect;
}

function isLocalTombstoneOverride() {
return self.ringpop.whoami() === self.address &&
update.status === Member.Status.tombstone;
}
return update.status === Member.Status.faulty ||
update.status === Member.Status.suspect ||
update.status === Member.Status.tombstone;
};

Member.prototype._isOtherOverride = function _isOtherOverride(update) {
var self = this;

return isAliveOverride() || isSuspectOverride() || isFaultyOverride() ||
isLeaveOverride() || isTombstoneOverride();

function isAliveOverride() {
return update.status === 'alive' &&
Member.Status[self.status] &&
update.incarnationNumber > self.incarnationNumber;
// update is newer than current member
if (update.incarnationNumber > self.incarnationNumber) {
return true;
}

function isFaultyOverride() {
return update.status === 'faulty' &&
((self.status === 'suspect' && update.incarnationNumber >= self.incarnationNumber) ||
(self.status === 'faulty' && update.incarnationNumber > self.incarnationNumber) ||
(self.status === 'tombstone' && update.incarnationNumber > self.incarnationNumber) ||
(self.status === 'alive' && update.incarnationNumber >= self.incarnationNumber));
}

function isLeaveOverride() {
return update.status === 'leave' &&
self.status !== Member.Status.leave &&
update.incarnationNumber >= self.incarnationNumber;
// update is older than current member
if (update.incarnationNumber < self.incarnationNumber) {
return false;
}

function isSuspectOverride() {
return update.status === 'suspect' &&
((self.status === 'suspect' && update.incarnationNumber > self.incarnationNumber) ||
(self.status === 'faulty' && update.incarnationNumber > self.incarnationNumber) ||
(self.status === 'tombstone' && update.incarnationNumber > self.incarnationNumber) ||
(self.status === 'alive' && update.incarnationNumber >= self.incarnationNumber));
}

function isTombstoneOverride() {
return update.status === 'tombstone' &&
((self.status === 'suspect' && update.incarnationNumber >= self.incarnationNumber) ||
(self.status === 'faulty' && update.incarnationNumber >= self.incarnationNumber) ||
(self.status === 'tombstone' && update.incarnationNumber > self.incarnationNumber) ||
(self.status === 'alive' && update.incarnationNumber >= self.incarnationNumber));

}
return Member.statusPrecedence(update.status) > Member.statusPrecedence(self.status);
};

Member.Status = {
Expand All @@ -259,4 +219,28 @@ Member.isStatusPingable = function isStatusPingable(status) {
}
};

/**
* Get the precedence of a status.
*
* @param {string} status a valid status (@see {Member.Status})
* @returns {number} A higher number would mean that the status is considered as a higher priority.
*/
Member.statusPrecedence = function statusPrecedence(status) {
switch (status) {
case Member.Status.alive:
return 0;
case Member.Status.suspect:
return 1;
case Member.Status.faulty:
return 2;
case Member.Status.leave:
return 3;
case Member.Status.tombstone:
return 4;
default:
// unknown states will never have precedence
return -1;
}
};

module.exports = Member;
13 changes: 7 additions & 6 deletions test/unit/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,11 +376,12 @@ test('emits ring changed event', function t(assert) {

var node1Addr = '127.0.0.1:3001';
var node2Addr = '127.0.0.1:3002';
var magicIncNo = Date.now() + 123456;
var incNo = Date.now();
var magicIncNo = incNo + 123456;

var ringpop = createRingpop();
ringpop.membership.makeAlive(ringpop.whoami(), Date.now());
ringpop.membership.makeAlive(node1Addr, Date.now());
ringpop.membership.makeAlive(ringpop.whoami(), incNo);
ringpop.membership.makeAlive(node1Addr, incNo);

function assertChanged(changer, intent) {
ringpop.once('membershipChanged', function onMembershipChanged() {
Expand All @@ -397,7 +398,7 @@ test('emits ring changed event', function t(assert) {
}

assertChanged(function assertIt() {
ringpop.membership.makeFaulty(node1Addr);
ringpop.membership.makeFaulty(node1Addr, incNo);
}, {
adding: [],
removing: [node1Addr]
Expand All @@ -406,8 +407,8 @@ test('emits ring changed event', function t(assert) {
assertChanged(function assertIt() {
ringpop.membership.makeAlive(node1Addr, magicIncNo);
}, {
adding: [],
removing: [node1Addr]
adding: [node1Addr],
removing: []
});

assertChanged(function assertIt() {
Expand Down
67 changes: 63 additions & 4 deletions test/unit/member_status_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ var _ = require('underscore');
var test = require('tape');

var Member = require('../../lib/membership/member');
var Ringpop = require('../../index');

var ALL_STATUSES = _.values(Member.Status);

test('is status pingable', function(assert) {
var expectation = {
Expand All @@ -34,10 +37,9 @@ test('is status pingable', function(assert) {
'tombstone': false
};

var statuses = _.values(Member.Status);
for (var i = 0; i < statuses.length; i++) {
var status = statuses[i];

for(var i=0; i<ALL_STATUSES.length; i++) {
var status = ALL_STATUSES[i];

if (expectation[status] !== undefined) {
assert.equal(Member.isStatusPingable(status), expectation[status], status + ' is ' + (expectation[status] ? '' : 'not ') + 'pingable');
} else {
Expand All @@ -52,3 +54,60 @@ test('is status pingable', function(assert) {

assert.end()
});

test('status precedence is correct', function t(assert) {
var precedenceOrder = [Member.Status.alive, Member.Status.suspect, Member.Status.faulty, Member.Status.leave, Member.Status.tombstone];

for (var i = 0; i < precedenceOrder.length; i++) {
var status = precedenceOrder[i];

for (var j = 0; j < i; j++) {
var otherStatus = precedenceOrder[j];

assert.true(Member.statusPrecedence(status) > Member.statusPrecedence(otherStatus), status + ' takes precedence over ' + otherStatus);
}
}
assert.end()
});

test('status precedence with unknown state never takes precedence', function t(assert) {
var unknownStatusPriority = Member.statusPrecedence('fake');

for(var k in Member.Status) {
var status = Member.Status[k];
assert.true(unknownStatusPriority < Member.statusPrecedence(status), 'unknown status does not take precedence over '+ status);
}
assert.end()
});

function testOtherOverride(currentState, expectedOverridingStatuses) {
test('test other override (' + currentState + ')', function t(assert) {
var ringpop = new Ringpop({app: 'test', hostPort: '127.0.0.1:3000'});

var member = new Member(ringpop, {
incarnationNumber: 1,
status: currentState
});

var overridingStatuses = [];

for (var i = 0; i < ALL_STATUSES.length; i++) {
var status = ALL_STATUSES[i];
if(member._isOtherOverride({incarnationNumber: 1, status: status})){
overridingStatuses.push(status);
}
assert.true(member._isOtherOverride({incarnationNumber: 2, status: status}), 'newer incarnation always overrides');
}

assert.deepEqual(overridingStatuses.sort(), expectedOverridingStatuses.sort());

ringpop.destroy();
assert.end();
});
}

testOtherOverride('alive', ['suspect', 'faulty', 'leave', 'tombstone']);
testOtherOverride('suspect', ['faulty', 'leave', 'tombstone']);
testOtherOverride('faulty', ['leave', 'tombstone']);
testOtherOverride('leave', ['tombstone']);
testOtherOverride('tombstone', []);

0 comments on commit 656d620

Please sign in to comment.