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

feature: add memcache client with consistent hashring #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

htbvo
Copy link

@htbvo htbvo commented Jun 14, 2019

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jun 14, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Huy Vo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jchip
Copy link
Member

jchip commented Jun 17, 2019

nice! impressive work.

Can you point me to the tests that verifies retrieving the same data would go to the same server?

Also, is there a test that checks and verifies if a server goes down?

  • Does it verify that some keys still go to remaining servers?
  • Does it verify that the keys on the removed server get redistributed?

Is there a test that verify adding a new server?

  • Does it verify that most existing keys remain as is?
  • Does it verify that the new server gets distribution?

By the way, I was thinking of doing this back in 2017.

I actually found a hashring implemented in C++ but outdated and I updated it to work.

https://github.com/jchip/node-hash-ring

Why is the bad server tests removed?

This module is used by teams at WalmartLabs for biz critical apps, so as long as existing code and behaviors are left untouched, I am happy to merge and release.

@htbvo
Copy link
Author

htbvo commented Jun 18, 2019

Hi! Thanks for the feedback, I'll add those test cases shortly. I just wanted to answer this question

Why is the bad server tests removed?

I wasn't able to get that test working node 8 and 10 just yet. As soon as I get that working again, I'll add it back. I created this PR a bit early to get feedback on the direction so far.

FYI, I had to change another test case too since the error that gets thrown is different depending on the version of node.

Were you able to run the tests on versions of node >= 8?

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