From c903a855385bb0924be983b73659f3aedf0fcf3d Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 18 Nov 2014 15:48:39 +0000 Subject: [PATCH 1/3] Listen on Sentinel PUBSUB channel for +switch-master --- index.js | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 71f5614..4d1f4c4 100644 --- a/index.js +++ b/index.js @@ -10,6 +10,8 @@ function Sentinel(endpoints) { } this.endpoints = endpoints; + this.clients = []; + this.subscribeToSentinelPubSub(); } /** @@ -33,6 +35,7 @@ Sentinel.prototype.createClient = function(masterName, opts) { var netClient = new net.Socket(); var client = new redis.RedisClient(netClient, opts); + this.clients.push(client); var self = this; @@ -112,6 +115,38 @@ Sentinel.prototype.createClient = function(masterName, opts) { return client; }; +/* + * Listen for '+switch-master' events from Sentinel, and reconnect all clients + * when received. + */ +Sentinel.prototype.subscribeToSentinelPubSub = function () { + this.endpoints.forEach(function(endpoint) { + client = redis.createClient(endpoint.port, endpoint.host); + client.subscribe("+switch-master", function(error) { + if (error) { + console.error("Unable to subscribe to Sentinel PUBSUB", endpoint); + } + }); + client.on("message", function(channel, message) { + console.warn("Received +switch-master message from Redis Sentinel. Reconnecting clients."); + sentinel.reconnectAllClients(); + }); + }); +} + +/* + * Ensure that all clients are trying to reconnect. + */ +Sentinel.prototype.reconnectAllClients = function() { + this.clients.forEach(function(client) { + // It is safe to call this multiple times in quick succession, as + // might happen with multiple Sentinel instances. Each client + // records its reconnect state and will only try to reconnect if + // not already doing so. + client.connection_gone("sentinel switch-master"); + }); +}; + function resolveClient() { var _i, __slice = [].slice; @@ -275,8 +310,13 @@ function parseSentinelResponse(resArr){ } // Shortcut for quickly getting a client from endpoints +var sentinel = null function createClient(endpoints, masterName, options) { - var sentinel = Sentinel(endpoints); + if (!sentinel) { + // Only create one global sentinel instance to bind to + // the Sentinel PUBSUB channel. + sentinel = Sentinel(endpoints); + } return sentinel.createClient(masterName, options); } From 942c0ce92d9c88a400da1cee339690a31cef5b01 Mon Sep 17 00:00:00 2001 From: Lenny Maiorani Date: Wed, 19 Nov 2014 15:25:20 -0700 Subject: [PATCH 2/3] Fix the Pub/Sub implementation to fix unit tests and not have a singleton. Problem: The original design of the sentinel with Pub/Sub to the switch-master message broke the last unit test because it was always connected to the previously successful servers. Analysis: In order to not have always stay connected to the old servers the subscription to the Pub/Sub message needs to only occur when the successful connect occurs. --- index.js | 54 +++++++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/index.js b/index.js index 4d1f4c4..02b9206 100644 --- a/index.js +++ b/index.js @@ -11,7 +11,7 @@ function Sentinel(endpoints) { this.endpoints = endpoints; this.clients = []; - this.subscribeToSentinelPubSub(); + this.pubsub = []; } /** @@ -21,7 +21,34 @@ function Sentinel(endpoints) { * @return {RedisClient} the RedisClient for the desired endpoint */ Sentinel.prototype.createClient = function(masterName, opts) { + // When the client is ready create another client and subscribe to the + // switch-master event. Then any time there is a message on the channel it + // must be a master change, so reconnect all clients. This avoids combining + // the pub/sub client with the normal client and interfering with whatever + // the user is trying to do. + if (this.pubsub.length == 0) { + var self = this; + var pubsubOpts = {}; + pubsubOpts.role = "sentinel"; + pubsubClient = this.createClientInternal(masterName, pubsubOpts); + pubsubClient.subscribe("+switch-master", function(error) { + if (error) { + console.error("Unable to subscribe to Sentinel PUBSUB", + host, ":", port); + } + }); + pubsubClient.on("message", function(channel, message) { + console.warn("Received +switch-master message from Redis Sentinel.", + " Reconnecting clients."); + self.reconnectAllClients(); + }); + pubsubClient.on("error", function(error) {}); + self.pubsub.push(pubsubClient); + } + return this.createClientInternal(masterName, opts); +} +Sentinel.prototype.createClientInternal = function(masterName, opts) { if (typeof masterName !== 'string') { opts = masterName; masterName = 'mymaster'; @@ -115,24 +142,6 @@ Sentinel.prototype.createClient = function(masterName, opts) { return client; }; -/* - * Listen for '+switch-master' events from Sentinel, and reconnect all clients - * when received. - */ -Sentinel.prototype.subscribeToSentinelPubSub = function () { - this.endpoints.forEach(function(endpoint) { - client = redis.createClient(endpoint.port, endpoint.host); - client.subscribe("+switch-master", function(error) { - if (error) { - console.error("Unable to subscribe to Sentinel PUBSUB", endpoint); - } - }); - client.on("message", function(channel, message) { - console.warn("Received +switch-master message from Redis Sentinel. Reconnecting clients."); - sentinel.reconnectAllClients(); - }); - }); -} /* * Ensure that all clients are trying to reconnect. @@ -310,13 +319,8 @@ function parseSentinelResponse(resArr){ } // Shortcut for quickly getting a client from endpoints -var sentinel = null function createClient(endpoints, masterName, options) { - if (!sentinel) { - // Only create one global sentinel instance to bind to - // the Sentinel PUBSUB channel. - sentinel = Sentinel(endpoints); - } + var sentinel = Sentinel(endpoints); return sentinel.createClient(masterName, options); } From 2acd9ac388404523aa0782954e747ad6f12a02ef Mon Sep 17 00:00:00 2001 From: Lenny Maiorani Date: Sat, 29 Nov 2014 11:09:41 -0700 Subject: [PATCH 3/3] Fix race condition in refresh by not connecting until resolver is complete. The resolver needs to run before the place to connect is determined, but node-redis will start attempting to reconnect immediately. Prevent this by setting the port and ip to the empty string and letting the resolver modify it. A better fix would be to modify node-redis to be able to not retry until told to do so. --- index.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 02b9206..5706626 100644 --- a/index.js +++ b/index.js @@ -97,11 +97,17 @@ Sentinel.prototype.createClientInternal = function(masterName, opts) { client.on('reconnecting', refreshEndpoints); function refreshEndpoints() { + client.connectionOption.port = ""; + client.connectionOption.host = ""; resolver(self.endpoints, masterName, function(_err, ip, port) { - if (_err) { oldEmit.call(client, 'error', _err); } - // Try and reconnect - client.connectionOption.port = port; - client.connectionOption.host = ip; + if (_err) { + oldEmit.call(client, 'error', _err); + } else { + // Try reconnecting. + client.connectionOption.port = port; + client.connectionOption.host = ip; + client.connection_gone("sentinel induced refresh"); + } }); }