Skip to content
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

Zen2: Add join validation #37203

Merged
merged 5 commits into from Jan 10, 2019

Conversation

Projects
None yet
4 participants
@ywelsch
Copy link
Contributor

commented Jan 7, 2019

Adds join validation to Zen2, which prevents a node from joining a cluster when the node does not have the right ES version or does not satisfy any other of the join validation constraints.

@elasticmachine

This comment has been minimized.

Copy link

commented Jan 7, 2019

@ywelsch

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@elasticmachine retest this please

@DaveCTurner
Copy link
Contributor

left a comment

Looks good. I left some nits.

@@ -277,6 +281,11 @@ PublishWithJoinResponse handlePublishRequest(PublishRequest publishRequest) {
+ lastKnownLeader + ", rejecting");
}

if (publishRequest.getAcceptedState().term() > coordinationState.get().getLastAcceptedState().term()) {
// only do join validation if we have not accepted state from this master yet
onJoinValidators.stream().forEach(a -> a.accept(getLocalNode(), publishRequest.getAcceptedState()));

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Jan 9, 2019

Contributor

Apparently the .stream() is unnecessary.

@@ -131,7 +131,7 @@ public DiscoveryModule(Settings settings, ThreadPool threadPool, TransportServic
discoveryTypes.put(ZEN2_DISCOVERY_TYPE, () -> new Coordinator(NODE_NAME_SETTING.get(settings), settings, clusterSettings,
transportService, namedWriteableRegistry, allocationService, masterService,
() -> gatewayMetaState.getPersistedState(settings, (ClusterApplierService) clusterApplier), hostsProvider, clusterApplier,
Randomness.get()));
joinValidators, Randomness.get()));

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Jan 9, 2019

Contributor

We say Collections.unmodifiableCollection(joinValidators) above. Should we also do so here?

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jan 9, 2019

Author Contributor

it's not necessary because it's wrapped again with addBuiltInJoinValidators. I will remove the unmodifiableCollection call above for uniformity.

Show resolved Hide resolved ...er/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java Outdated
Show resolved Hide resolved server/src/main/java/org/elasticsearch/cluster/coordination/JoinHelper.java Outdated
Show resolved Hide resolved server/src/main/java/org/elasticsearch/cluster/coordination/JoinHelper.java Outdated
Show resolved Hide resolved server/src/main/java/org/elasticsearch/cluster/coordination/JoinHelper.java Outdated
Show resolved Hide resolved server/src/main/java/org/elasticsearch/cluster/coordination/JoinHelper.java Outdated
Show resolved Hide resolved ...c/test/java/org/elasticsearch/cluster/coordination/CoordinatorTests.java Outdated

if (stateForJoinValidation.nodes().isLocalNodeElectedMaster()) {
// we do this in a couple of places including the cluster update thread. This one here is really just best effort
// to ensure we fail as fast as possible.

This comment has been minimized.

Copy link
@DaveCTurner

DaveCTurner Jan 9, 2019

Contributor

Is this true? I see that we call ensureIndexCompatibility and ensureNodesCompatibility in JoinTaskExecutor, but I don't see that we call any other join validators there.

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jan 9, 2019

Author Contributor

copied this from ZenDiscovery. It's obviously wrong. I've moved the comment to the call of ensureMajorVersionBarrier, to which is actually applies

ywelsch added some commits Jan 9, 2019

@ywelsch ywelsch requested a review from DaveCTurner Jan 9, 2019

@ywelsch

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

Merged latest master, as FlushIT test failed because missing #37229

@ywelsch ywelsch referenced this pull request Jan 9, 2019

Closed

A new cluster coordination layer #32006

52 of 61 tasks complete
@DaveCTurner
Copy link
Contributor

left a comment

LGTM

@ywelsch ywelsch merged commit d499233 into elastic:master Jan 10, 2019

7 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci-1 Build finished.
Details
elasticsearch-ci-2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

javanna added a commit that referenced this pull request Jan 10, 2019

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.