-
Notifications
You must be signed in to change notification settings - Fork 113
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 seekLeaderStore NPE #366
fix seekLeaderStore NPE #366
Conversation
Signed-off-by: iosmanthus <[email protected]>
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
/run-all-tests |
@iosmanthus please remove the necessary checks in PR description |
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.
could you add some tests?
It is really hard to inject the error to trigger a double retry of seekLeaderStore. We can only use integration to cover this case. |
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
Signed-off-by: ti-srebot <[email protected]>
cherry pick to release-3.1 in PR #371 |
Signed-off-by: Ankita Wagh <[email protected]>
What problem does this PR solve?
This pull request fixes the NPE triggered by the double retry of
seekLeaderStore
. These buggy lines would also introduce another issue likepeer id not match
because the meta of the region is not updated.What is changed and how it works?
Once we change detected a reachable store we need to update the peer information of the region and retry in the most outside code.
Check List
Tests
Code changes
Side effects
Related changes