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.

Sign up for GitHub

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

Jump to bottom

Introduce universal bootstrap method for tests with autoMinMasterNodes=false #38038

Merged
merged 11 commits into from
Feb 1, 2019
Merged

Introduce universal bootstrap method for tests with autoMinMasterNodes=false #38038

merged 11 commits into from
Feb 1, 2019

Conversation

andrershov
Copy link
Contributor

Currently, there are a few tests that use autoMinMasterNodes=false and
hence override addExtraClusterBootstrapSettings, mostly this is 10-30
lines of codes that are copy-pasted from class to class.

This PR introduces bootstrapMasterNodeWithSpecifiedId which is
suitable for all classes and copy-paste could be removed.

Removing code is always a good thing!

@andrershov andrershov added >test Issues or PRs that are addressing/adding tests :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Jan 30, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

DaveCTurner
DaveCTurner reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Great, the duplication irked me too.

I think it can be further simplified, moving this whole mechanism within InternalTestCluster. I also wonder if we should reset bootstrapMasterNodeId to -1 after use, and should assert that it is -1 if autoManageMinMasterNodes is set.

Also there's a few places where you org.elasticsearch.use org.elasticsearch.fully-qualified org.elasticsearch.names, mirroring existing usage. I think we can tidy that up, at least in the places they occur here. There's only one NODE_NAME_SETTING worth talking about.

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java Outdated
* See {@link #addExtraClusterBootstrapSettings(List)}.
* Please note that this value is being reset to -1 after each test. See {@link #resetBootstrapMasterNodeId()}.
*/
protected int bootstrapMasterNodeId = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this entirely within InternalTestCluster?

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java Outdated
@@ -1986,17 +1998,63 @@ public Settings transportClientSettings() {
};
}

/**
* Performs cluster bootstrap when {@link #bootstrapMasterNodeId} is started with the names of all
* previously started master-eligible nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/previously started/existing and new/?

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java Outdated
/**
* This method is called before starting a collection of nodes.
* At this point the test has a holistic view on all nodes settings and might perform settings adjustments as needed.
* For instance, the test could retrieve master node names and fill in
* {@link org.elasticsearch.cluster.coordination.ClusterBootstrapService#INITIAL_MASTER_NODES_SETTING} setting.
* By default, this method delegates to {@link #bootstrapMasterNodeWithSpecifiedId(List)} method.
*
* @param allNodesSettings list of node settings before update
* @return list of node settings after update
*/
protected List<Settings> addExtraClusterBootstrapSettings(List<Settings> allNodesSettings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is never overridden, so can be inlined.

Andrey Ershov added 3 commits January 30, 2019 17:40
Remove unused import
f7046e4
Removed f.q. usage, assert autoMinMasterNodes=false,
3d0ffed 
reset bootstrapMasterNodeId after bootstrap
Merge branch 'master' into zen2_tests_bootstrap
738cf9f 
# Conflicts:
#	server/src/test/java/org/elasticsearch/cluster/MinimumMasterNodesIT.java
#	server/src/test/java/org/elasticsearch/cluster/SpecificMasterNodesIT.java
#	server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapMasterIT.java
@andrershov
Copy link
Contributor Author

@DaveCTurner thanks for your review!
And thank you for the suggestion to move bootstrap logic to InternalTestCluster, after this change addExtraClusterBootrstrap method could be removed from NodeConfigurationSource and ESIntegTestCase. If in the future, we will require a method to add some settings before nodes are started, it can easily be returned back.
I've made all the requested changes and I think it makes sense to have the second review round.

@andrershov
Copy link
Contributor Author

I've missed that InternalTestClusterTests also overrides addExtraClusterBootstrapSettings. Now it can happily go away.

DaveCTurner
DaveCTurner approved these changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM, nice work

@andrershov andrershov merged commit bfd618c into elastic:master Feb 1, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 1, 2019
@jasontedor
Merge branch 'master' into retention-leases-version
7cf145c 
* master: (36 commits)
  Ensure joda compatibility in custom date formats (elastic#38171)
  Do not compute cardinality if the `terms` execution mode does not use `global_ordinals` (elastic#38169)
  Do not set timeout for IndexRequests in GatewayIndexStateIT (elastic#38147)
  Zen2ify testMasterFailoverDuringIndexingWithMappingChanges (elastic#38178)
  SQL: [Docs] Add limitation for aggregate functions on scalars (elastic#38186)
  Add elasticsearch-node detach-cluster command (elastic#37979)
  Add tests for fractional epoch parsing (elastic#38162)
  Enable bw tests for elastic#37871 and elastic#38032. (elastic#38167)
  Clear send behavior rule in CloseWhileRelocatingShardsIT (elastic#38159)
  Fix testCorruptedIndex (elastic#38161)
  Add finalReduce flag to SearchRequest (elastic#38104)
  Forbid negative field boosts in analyzed queries (elastic#37930)
  Remove AtomiFieldData#getLegacyFieldValues (elastic#38087)
  Universal cluster bootstrap method for tests with autoMinMasterNodes=false (elastic#38038)
  Fix FullClusterRestartIT.testHistoryUUIDIsAdded (elastic#38098)
  Replace joda time in ingest-common module (elastic#38088)
  Fix eclipse config for ssl-config (elastic#38096)
  Don't load global ordinals with the `map` execution_hint (elastic#37833)
  Relax fault detector in some disruption tests (elastic#38101)
  Fix java time epoch date formatters (elastic#37829)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees
No one assigned
Labels
:Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >test Issues or PRs that are addressing/adding tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrershov @elasticmachine @DaveCTurner