Skip to content

[Zen2] Migrate no-master-block integration tests #36502

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

Conversation

DaveCTurner
Copy link
Contributor

This change follows up on #36478 by migrating the affected integration tests to
use Zen2.

This change follows up on elastic#36478 by migrating the affected integration tests to
use Zen2.
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests v7.0.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Dec 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@andrershov andrershov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except Zen setting, which ideally should not be used.

.put(DiscoverySettings.NO_MASTER_BLOCK_SETTING.getKey(), "all")
.build();
.put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), true)
.put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), Integer.MAX_VALUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get rid of this setting, because we're running Zen2 and not Zen? Or should it stay until Zen is completely removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately today InternalTestCluster currently requires that it's explicitly set if autoMinMasterNodes is false, which is why it's present but set ludicrously high. Once we've finished migrating everything to Zen2 it will be easy enough to find all usages like this to remove them.

.put(DiscoverySettings.NO_MASTER_BLOCK_SETTING.getKey(), "write")
.build();
.put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), false)
.put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), Integer.MAX_VALUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get rid of this setting, because we're running Zen2 and not Zen? Or should it stay until Zen is completely removed?

@DaveCTurner
Copy link
Contributor Author

@elasticmachine please run the Gradle build tests 2

.build();
.put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), true)
.put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), Integer.MAX_VALUE)
.put(GatewayService.RECOVER_AFTER_MASTER_NODES_SETTING.getKey(), 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add this setting? This should not be needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It defaults to whatever minimum_master_nodes is. I could just set that to 3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah that's annoying, and does not make any sense in Zen2. Can you perhaps change GatewayService to not do this if discovery instanceof Coordinator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I pushed b0e9643.

.build();
.put(AutoCreateIndex.AUTO_CREATE_INDEX_SETTING.getKey(), false)
.put(ElectMasterService.DISCOVERY_ZEN_MINIMUM_MASTER_NODES_SETTING.getKey(), Integer.MAX_VALUE)
.put(GatewayService.RECOVER_AFTER_MASTER_NODES_SETTING.getKey(), 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. Why is this needed?

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DaveCTurner
Copy link
Contributor Author

Apparently-unrelated failures.

@elasticmachine please run the gradle build tests 1

@DaveCTurner DaveCTurner merged commit aa43e0b into elastic:master Dec 12, 2018
@DaveCTurner DaveCTurner deleted the 2018-12-11-no-master-block-integration-tests branch December 12, 2018 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >test Issues or PRs that are addressing/adding tests v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants