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

Add the InstanceInfo to the response in case of 409 status code #934

Merged

Conversation

jebeaudet
Copy link
Contributor

Fixes #933.

Thanks!

When the server receive a heartbeat from a peer with a lastDirtyTimestamp lower
than the timestamp it has, it will return a 409 Conflict response code. On the
client side, it will use this status code to trigger a resync on this particular
instance.

The PeerReplicationResource was however stripping the required InstanceInfo
from the response unless the status code was a 200 which is not the case here.
This simple fix adds the InstanceInfo to the response in case of 409.

Fixes Netflixgh-933
@qiangdavidliu
Copy link
Contributor

Thanks @jebeaudet, taking a look ...

@jebeaudet
Copy link
Contributor Author

@qiangdavidliu Have you had the chance to give it a look? We're releasing this patch in our production environment soon and I'd sleep better if I had your green thumbs on it 😉

@qiangdavidliu
Copy link
Contributor

Hi @jebeaudet apologies for the delayed response. This PR looks sound and makes logical sense, but I want to set up a quick test in our test env to test this out just to verify any potential unintended side effects. I'll let you know soon.

@qiangdavidliu
Copy link
Contributor

qiangdavidliu commented May 31, 2017

@jebeaudet prelim trial seems ok, though there were some weirdness. Will merge this and I'll also set up a config guard for a quick 'undo'. Thanks for the contrib!

@qiangdavidliu qiangdavidliu merged commit f0ad29d into Netflix:master May 31, 2017
qiangdavidliu added a commit that referenced this pull request Jun 1, 2017
@jebeaudet
Copy link
Contributor Author

@qiangdavidliu Did you get a chance to test it enough to remove the failguard of 6f3f997?

@jebeaudet
Copy link
Contributor Author

@qiangdavidliu Any info about that safeguard? 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.

2 participants