-
Notifications
You must be signed in to change notification settings - Fork 603
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(raft): notify role change listener only when transition completed #8285
Conversation
f592c7a
to
44f62d9
Compare
This PR implements the potential solution we have discussed to avoid calling the role change listener twice. While the solution works in general, I am still wondering if there are any alternative solutions you could think of? Why am I asking? With changing the behavior of Before that change, it behaved pretty deterministically:
With the PR, now the behavior changes in a way that the listener gets called by the previous leader and not by the new leader. So while the test case itself is still fine - it still gets green, the code change potentially changes the semantic of this (and maybe other) test case(s). Of course, I could now go through the test cases and adjust them accordingly 😄 But maybe there is an alternative you could think of? |
What do you think about the following approach? Basically, instead of pushing the case that the listener might be called twice to the Raft layer, the concern/issue should stay within the broker itself. Meaning, keep the way of registering and triggering the listener as it is in Basically, if the current role is equal to the role to transition to, then do nothing and return a completed future. In my opinion, this should not cause any issues also when calling follow-up listener like That way, we keep the "issue" and its handling in the broker and don't push it to the Raft layer. What do you think? |
Having a check in ZeebePartition to prevent multiple transitions to the same Role is a good idea. But I think this will not solve the issue completely. This will make sure that the second transition will not happen. But the main problem here is that the ZeebePartition is transitioning to Leader before InitialEntry is committed, because it actively queries Raft's current role, thus bypassing the listener notification. So in this case it can still happen that in the first transition, it reads the wrong lastEntry, because the entries are not committed yet. This will leads to the same error before the second transition happens. So we have to anyway remove the following call
from |
@deepthidevaki, true, you are right! |
I cannot think of alternative solutions. Are there too many tests that needs to be adjusted because of this change? |
f1ac75f
to
88a4aec
Compare
@deepthidevaki, thanks for your input and the quick discussion. Just a quick summary of what we have talked about:
|
I am done with the implementation of the fix. I tried to write a test case, therefore, but I couldn't find a way to reproduce the scenario in a test case and to control for example when the commit happens should happen. I think, the current test cases (i.e., the one that I adjusted a bit) implicitly ensure that the listener is notified correctly and especially in the case of a leader when the initial entry gets committed. However, if you have an approach, I am happy to receive it! |
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.
🚀 LGTM.
May be we can also add a test to verify that the listener is notified after it is added, even if there was no role change. This is the semantics changed in this PR, right.
88a4aec
to
d05f984
Compare
@deepthidevaki, sure thing! |
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.
d05f984
to
9ae3cea
Compare
bors merge |
Build succeeded: |
Backport failed for Please cherry-pick the changes locally. git fetch origin stable/1.2
git worktree add -d .worktree/backport-8285-to-stable/1.2 origin/stable/1.2
cd .worktree/backport-8285-to-stable/1.2
git checkout -b backport-8285-to-stable/1.2
ancref=$(git merge-base a36abef9d3d90ba000e169e03138bc4f4284fca1 9ae3cea7e69bdd8cf55b226e973c8933b9e6fe52)
git cherry-pick -x $ancref..9ae3cea7e69bdd8cf55b226e973c8933b9e6fe52 |
8312: [Backport stable/1.2] fix(raft): notify role change listener only when transition completed r=deepthidevaki a=romansmirnov ## Description backports #8285 <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> relates to #7862 Co-authored-by: Roman <roman.smirnov@camunda.com>
Description
ZeebePartition#onRoleChange()
when registering the role change listenerRelated issues
closes #7862
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/0.25
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation: