Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Controlling discovery for decommissioned nodes #4590
Controlling discovery for decommissioned nodes #4590
Changes from all commits
a015a81
0a3f262
24fcaaf
c9e0f5b
bb0547d
2ea4dc9
2582c4b
70b7d48
a30b83d
da2fa40
61bb893
d872065
bee48b4
d02f9ae
6c0ce6c
01f2d70
8f82360
1c497b7
e4d1354
1ac690d
f448d78
adc4a8c
69f2784
c42586b
a53e578
edff382
46c49b0
63f6c00
b8cf3fe
6a4c16f
50a6e67
c9db6b1
bb6f573
8dd4457
19c524d
c5b1c0d
41cc099
260641a
8e161a6
4a932b7
05d073e
3b6c1b7
8b94b8d
165b961
e4e1b5c
0b3f106
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The current flow is:
The above logic will not work if the current master eligible node is not active leader and is a candidate accumulating joins Code Ref
Suggestion:
Ideally the decommissioning check should be executed at
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.
In that case, can you please help me understand that when current leader eligible node is not active leader and is just accumulating joins, we just skip the validator code flow?
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 for pointing this out @shwetathareja. As per your suggestions, implementing the changes
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.
Made these changes.
STATE_NOT_RECOVERED_BLOCK
)@shwetathareja , lmk if this looks ok
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.
Hey, I explored more on join validators -
I think we can keep it as built in join validators, and Coordinator#handleJoinRequest validates all the checks here. So handle join request would be covered here. Another case I see where keeping it at BuiltinValidator would help is during handlePublishRequest. Ideally we would never hit this case as it runs only on Mode.Leader but I think for any such cases in future, it would be good to have all the validators at one place
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.
Built validator delays the check which can be checked early on during handleJoinRequest itself. Can you help me understand the handlePublishRequest scenario?
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.
This is the snippet from handleJoinRequest of Coordinator. If you see, this method is also running the same built in validators
onJoinValidators.forEach(a -> a.accept(joinRequest.getSourceNode(), stateForJoinValidation));
. And hence the concern with early checking is actually already resolved hereThere 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.
We might never actually hit the
handlePublishRequest
scenario as this would be run only forLeader
mode. But what I was trying to say is plugins put their join validators inonJoinValidators
and then Coordinator adds all other validators to thisonJoinValidators
. And then in handlePublishRequest and handleJoinRequest we validate with whatever validatorsonJoinValidators
has. Hence, keeping all the validators centrally inonJoinValidators
might be goodThere 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.
handlePublishRequest is executed on the node joining the master. But with the check inside
handleJoinRequest
master will reject the joins upfront.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.
Correct. I meant the built in validators are also executed in handleJoinRequest
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.
Add test for 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.
Tests for these were added as part of #4084. Adding it below again for ref -
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.
Please add test in
JoinTaskExecutorTests
class to ensure this exist in the executor method.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.
Above mentioned tests are in
JoinTaskExecutorTests
. Added one more to test validator in execute method