Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cluster): add the option for a custom node selector in scaleReads #252

Merged
merged 1 commit into from
Feb 10, 2016
Merged

feat(cluster): add the option for a custom node selector in scaleReads #252

merged 1 commit into from
Feb 10, 2016

Conversation

ramonsnir
Copy link

@luin
Copy link
Collaborator

luin commented Feb 10, 2016

Awesome! Thank you for the pull request!

It seems better to change function(slaves, command) {} to function(nodes, command) {} where the first element of nodes is the master node and the reset are slaves so that it's possible to send the queries to masters.

return _this.connectionPool.nodes.all[key];
});
redis = to(nodes, command);
if (redis.constructor === Array) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (Array.isArray(redis)) may be better IMO, and we should check whether redis is null/undefined and default to the master node.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct. I'm sadly used too much to client-side JS, and Array.isArray is unavailable in some MSIE versions...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a hint: there's a easy polyfill for this and Microsoft itself does not maintain IE 8 or older, not even with security fixes on any regular Windows version. Most libraries drop support for it or have already dropped the support and the faster all developers switch to not supporting it anymore, the faster no one is going to visit any website with it anymore.

@luin
Copy link
Collaborator

luin commented Feb 10, 2016

LGTM. Any thoughts about changing function(slaves, command) {} to function(nodes, command) {} so that we don't need to call slice(1) everytime.

@ramonsnir
Copy link
Author

It bothered me too... I'll do that, and make sure the README states it clearly.

luin added a commit that referenced this pull request Feb 10, 2016
feat(cluster): add the option for a custom node selector in scaleReads
@luin luin merged commit ca9ffbd into redis:2.x Feb 10, 2016
@luin
Copy link
Collaborator

luin commented Feb 10, 2016

Cool! Merged. 👍

@ramonsnir
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants