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

perf(cluster): improve the performance of calculating slots #323

Merged
merged 1 commit into from
Jun 12, 2016

Conversation

luin
Copy link
Collaborator

@luin luin commented Jun 9, 2016

@luin luin force-pushed the perf/cluster-key-slot branch from 8b7af16 to 13061d7 Compare June 9, 2016 10:30
@shaharmor
Copy link
Collaborator

LGTM

@Salakar
Copy link
Collaborator

Salakar commented Jun 10, 2016

Just one small change that needs doing, the pipeline can use var calculateSlotMulti = require('cluster-key-slot').generateMulti; to calculate the slots for the array of keys there, then instead of checking for 'undefined' you just check if slot < 0 (returns -1 if any slot isn't the same) - then throw the 'keys must be of same slot' error.

The multi on the calc repo is a little bit more performant. https://github.com/Salakar/cluster-key-slot#usage

@Salakar
Copy link
Collaborator

Salakar commented Jun 10, 2016

Hmm build test failures seem a little random and unrelated?

@luin luin force-pushed the perf/cluster-key-slot branch from 2a39cb1 to e7b9ae2 Compare June 10, 2016 14:37
@luin luin force-pushed the perf/cluster-key-slot branch from e7b9ae2 to fd1b28b Compare June 10, 2016 15:01
@luin
Copy link
Collaborator Author

luin commented Jun 10, 2016

@Salakar Just fixed it. If one of the test failed, the connection to the server will not be disconnected, so other tests will also fail.

@Salakar
Copy link
Collaborator

Salakar commented Jun 11, 2016

Looks good, LGTM =]

@luin luin merged commit 3ab4e8a into master Jun 12, 2016
@luin luin deleted the perf/cluster-key-slot branch June 12, 2016 06:43
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