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
Don't join master nodes or accept join requests of old and too new nodes #11972
Conversation
Settings nodeSettings = Settings.settingsBuilder() | ||
.put("discovery.type", "zen") // <-- To override the local setting if set externally | ||
.build(); | ||
String nodeName = internalCluster().startNode(nodeSettings, Version.V_1_6_0); |
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 wonder how long this would work ;)
Left some comments.. |
@bleskes thanks for the feedback! I updated the PR. Instead of moving the version check to findMaster(), I moved it to ElectMasterService#electMaster(), so the check doesn't need to get duplicated multiple times. |
|
||
if (!transportService.addressSupported(node.address().getClass())) { | ||
// TODO, what should we do now? Maybe inform that node that its crap? | ||
logger.warn("received a wrong address type from [{}], ignoring...", node); | ||
} else { | ||
// Sanity checks: likely we don't end up here, because serialization is likely to fail. |
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.
why is serailization likely to fail? it might, it might not...
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.
maybe my choice of wording here... I'll change it to: we may not end up here.....
Thx @martijnvg . I left some minor comments. |
@bleskes cool, I've updated the PR. |
@bleskes I change |
@bleskes I reverted the previous commit and let |
@@ -886,12 +890,20 @@ static boolean shouldIgnoreOrRejectNewClusterState(ESLogger logger, ClusterState | |||
} | |||
} | |||
|
|||
private void handleJoinRequest(final DiscoveryNode node, final MembershipAction.JoinCallback callback) { | |||
void handleJoinRequest(final DiscoveryNode node, final MembershipAction.JoinCallback callback) { |
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 can go back to private now, no?
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.
true
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.
oops... no, because it is used in a test (ZenDiscoveryTests#testHandleNodeJoin_incompatibleMinVersion)
LGTM. Left a couple of trivial comments. No need for another review. |
026a41e
to
72aad3a
Compare
…ew nodes. If the version of a node is lower than the minimum supported version or higher than the maximum supported version, a node shouldn't be allowed to join and nodes should join that elected master node Closes elastic#11924
72aad3a
to
a4b99e6
Compare
If the version of a node is lower than the minimum supported version or higher than the maximum hypothetical supported version, a node shouldn't be allowed to join and nodes should join that elected master node
Closes #11924