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
[ZOOKEEPER-3471] Fix potential lock unavailable due to dangling ephemeral nodes left during local session upgrading #1025
Conversation
@@ -992,6 +990,7 @@ public void submitRequestNow(Request si) { | |||
touch(si.cnxn); | |||
boolean validpacket = Request.isValid(si.type); | |||
if (validpacket) { | |||
setLocalSessionFlag(si); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing some info here (Please correct me if I am). So this happening only for requests going through RequestThrottler? And not the requests going through RequestProcessor? (Since this submitRequestNow
function is being called only in RequestThrottler)
Appreciate if you could explain little bit more. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure I understand your question here, all the requests will go through RequestThrottler, and then being put into RequestProcessor now.
So no request will be put into RequestProcessor directly, right? Let me know if I didn't get your question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lvfangmin . I think I misunderstood. It was good to clarify and I checked little deeper after that. Yes, now I understood all the requests go through RequestThrottler
.
And the change looks good to me. Thanks for fixing it.
…uring local session upgrading
4be4dd9
to
8eacd11
Compare
I left a comment in JIRA a while ago. Basically, the change looks fine but I failed to reason about the exact race condition and why this change could fix it, thus was asking a little bit clarification in JIRA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@hanm sorry for the lately reply, was fighting for a serials of prod issues due to strange JVM AVX bugs. I've added the detailed flow about the race condition in the JIRA, let me know if it's not clear. Checked the Jenkins, it's failed due to a flaky metric test, this PR shouls be ready to go. @eolivelli @hanm @anmolnar please help merge it if it looks good. |
Committed to master branch. Thanks @lvfangmin ! |
@lvfangmin |
…ral nodes left during local session upgrading Author: Fangmin Lyu <fangmin@apache.org> Reviewers: andor@apache.org Closes apache#1025 from lvfangmin/ZOOKEEPER-3471
…ral nodes left during local session upgrading Author: Fangmin Lyu <fangmin@apache.org> Reviewers: andor@apache.org Closes apache#1025 from lvfangmin/ZOOKEEPER-3471
…ral nodes left during local session upgrading Author: Fangmin Lyu <fangmin@apache.org> Reviewers: andor@apache.org Closes apache#1025 from lvfangmin/ZOOKEEPER-3471
No description provided.