Repository: luin/ioredis
Pull request #441 (2017-03-17 15:59:27)

Add a default port for sentinel connections.
Also add a few helpful notes to the README. This came from some frustrations I had the first time I setup a project to talk to sentinels.

reconbot: Updated the PR from the review notes.
reconbot: I didn't understand that fact. I thought sentinels overrode it. I was
coming from a project that used urls.

On Mon, Mar 20, 2017, 12:52 PM Zihua 子骅 <notifications@github.com> wrote:

> *@luin* commented on this pull request.
> ------------------------------
>
> In README.md
> <https://github.com/luin/ioredis/pull/441#discussion_r106954344>:
>
> >
>  To connect using Sentinel, use:
>
>  ```javascript
>  var redis = new Redis({
>    sentinels: [{ host: 'localhost', port: 26379 }, { host: 'localhost', port: 26380 }],
> -  name: 'mymaster'
> +  name: 'mymaster',
> +  db: 0
>
> db option is not sentinel-specified and it's documented here:
> https://github.com/luin/ioredis/blob/master/API.md#new-redisport-host-options.
> What problem did you encounter?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/luin/ioredis/pull/441#discussion_r106954344>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AABlblNwaAAtru3WftqJ95YB5Ycsb8YZks5rnq6vgaJpZM4MgxnI>
> .
>

luin: Thank you for the pull request! All look good to me except the `db: 0` part. I'm afraid setting it explicitly will lead people into thinking `db` is required even if they just want to connect to the first database.
reconbot: Alrighty - I'd rather put `// optional` but I'm not strong on it. Updated PR!

Patch:
diff --git a/README.md b/README.md
index 37030ab..1c7c0f2 100644
--- a/README.md
+++ b/README.md
@@ -590,7 +590,7 @@ var redis = new Redis({
 
 ## Sentinel
 ioredis supports Sentinel out of the box. It works transparently as all features that work when
-you connect to a single node also work when you connect to a sentinel group. Make sure to run Redis >= 2.8.12 if you want to use this feature.
+you connect to a single node also work when you connect to a sentinel group. Make sure to run Redis >= 2.8.12 if you want to use this feature. Sentinels have a default port of 26379.
 
 To connect using Sentinel, use:
 
diff --git a/lib/connectors/sentinel_connector.js b/lib/connectors/sentinel_connector.js
index 7b863d4..ff486b4 100644
--- a/lib/connectors/sentinel_connector.js
+++ b/lib/connectors/sentinel_connector.js
@@ -210,7 +210,7 @@ SentinelConnector.prototype.resolve = function (endpoint, callback) {
     Redis = require('../redis');
   }
   var client = new Redis({
-    port: endpoint.port,
+    port: endpoint.port || 26379,
     host: endpoint.host,
     retryStrategy: null,
     enableReadyCheck: false,
@@ -232,7 +232,7 @@ function noop() {}
 
 function isSentinelEql(a, b) {
   return ((a.host || '127.0.0.1') === (b.host || '127.0.0.1')) &&
-    ((a.port || 6379) === (b.port || 6379));
+    ((a.port || 26379) === (b.port || 26379));
 }
 
 module.exports = SentinelConnector;


Test patch:
diff --git a/test/functional/sentinel.js b/test/functional/sentinel.js
index b8e6635..a46ff54 100644
--- a/test/functional/sentinel.js
+++ b/test/functional/sentinel.js
@@ -18,6 +18,22 @@ describe('sentinel', function () {
 
     });
 
+    it('should default to the default sentinel port', function (done) {
+      var sentinel = new MockServer(26379);
+      sentinel.once('connect', function () {
+        redis.disconnect();
+        sentinel.disconnect(done);
+      });
+
+      var redis = new Redis({
+        sentinels: [
+          { host: '127.0.0.1' }
+        ],
+        name: 'master'
+      });
+
+    });
+
     it('should try to connect to all sentinel', function (done) {
       var sentinel = new MockServer(27380);
       sentinel.once('connect', function () {