Add a default port for sentinel connections. #441
Conversation
README.md
Outdated
|
|
||
| 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 |
There was a problem hiding this comment.
db defaults to 0 so why we add it here?
There was a problem hiding this comment.
As a new user to sentinels and not finding any docs for it in the api.md having this clue would have saved me a bit of headache.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I didn't understand that fact. I thought sentinels overrode it. I was coming from a project that used urls.
lib/connectors/sentinel_connector.js
Outdated
| this.sentinels = this.options.sentinels.map(function (endpoint) { | ||
| endpoint.port = endpoint.port || 26379; | ||
| return endpoint; | ||
| }); |
There was a problem hiding this comment.
Updating properties of the option introducing side effects. For example:
const sentinels: [{host: '192.168.1.3'}]
const redis = new Redis({sentinels})
console.log(sentinels) // values changed unexpectedlyWhat about give the default port when creating a new instance by port: endpoint.port || 26379: https://github.com/luin/ioredis/blob/master/lib/connectors/sentinel_connector.js#L213.
There's an isSentinelEql function should be updated accordingly.
There was a problem hiding this comment.
We could copy the sentinel objects, but I get your point. I'll patch.
reconbot
force-pushed
the
sentinel-default-ports
branch
from
0ca44bd to
44a8967
Compare
|
Updated the PR from the review notes. |
|
Thank you for the pull request! All look good to me except the |
Also add a few helpful notes to the README
reconbot
force-pushed
the
sentinel-default-ports
branch
from
44a8967 to
b78e692
Compare
|
Alrighty - I'd rather put |
|
Aha! Merged! 🍻 |
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.