-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix the inconsistency logic in method updateRenewalThreshold(). #1071
Conversation
…intention based on the comment in the method of updateRenewalThreshold().
@neoremind thanks |
@holy12345 thanks for your reply. I am working on adding test case for the scenario, but before that I got one important question to confirm #1073. Feel free to add comments. |
@holy12345 @qiangdavidliu I will clarify briefly about what I have done. I create a new testcase named
After reviewing the history of method |
@qiangdavidliu would you please review this PR? I suppose you are busy working, but still really appreciate if you could take a look. many thanks. :-) |
Thanks @neoremind . I'm a little busy right now, but I'll try to take a look at this when I can. |
@qiangdavidliu Thanks very much! Take your time. |
I want to more talk about this issue. In my mind, if client has not send heartbeat,eureka server will remove it. but during remove it , Eureka server why not update those number .(Eureka server use a background task do update two numbers). I understand this is Eureka architect, but can you explain more why design like this. I will appreciate you answer. Thanks |
@holy12345 As far as I know, Eureka will update
But in expired cases, the value will not be updated since self-preservation relies on the value to determine if heartbeats drop below 85% of the expected number, so Eureka should not update this value when removing expired instances. |
Thanks for you answer, but in my mind |
OK I get it.
is the reason. |
@holy12345 you got it :) |
Oh good. Thank you! |
@qiangdavidliu Sorry to bring you back, would you please review this PR and test case? I think this could help a lot on the core logic verification. Many thanks! |
All guys Great thanks @qiangdavidliu |
@neoremind @holy12345 apologies on not being able to take a look at this, we have been swamped and have not had the bandwidth to work on this. To answer your question about how we are using this, we are using the existing core logic as is, and have not had any issues with it as far as we can remember. |
I've seen a situation similar to this in which we had the renewal threshold at 85%. We took down an AZ which resulted in roughly 33% of the registrations disappearing, since this was higher than the threshold, the servers entered self preservation mode and were not able to exit this mode since the update threshold would not update as the services were rescheduled on the remaining AZs. |
@mattnelson In normal case, |
@qiangdavidliu Understand the situation that you are overwhelmed, Netflix grows so fast 👍 If this logic seems unnecessary, how about only adding the test case to eureka? |
@neoremind have looked over your changes over the weekend, these look good, and I very much appreciate the test cases attached. I'll merge this and cut a new release. |
@qiangdavidliu many thanks! Appreciate your help! :) |
Fix the inconsistency logic in method
updateRenewalThreshold()
. Because it does not reflect the expected intention based on the comment.updateRenewalThreshold
will be qualified to run if renewal count * 2 is bigger thanNOT