Introduce universal bootstrap method for tests with autoMinMasterNodes=false#38038
Introduce universal bootstrap method for tests with autoMinMasterNodes=false#38038andrershov merged 11 commits intoelastic:masterfrom
Conversation
andrershov
added
>test
|
Pinging @elastic/es-distributed |
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
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.
| * See {@link #addExtraClusterBootstrapSettings(List)}. | ||
| * Please note that this value is being reset to -1 after each test. See {@link #resetBootstrapMasterNodeId()}. | ||
| */ | ||
| protected int bootstrapMasterNodeId = -1; |
There was a problem hiding this comment.
Can we move this entirely within InternalTestCluster?
|
|
||
| /** | ||
| * Performs cluster bootstrap when {@link #bootstrapMasterNodeId} is started with the names of all | ||
| * previously started master-eligible nodes. |
There was a problem hiding this comment.
s/previously started/existing and new/?
| * @param allNodesSettings list of node settings before update | ||
| * @return list of node settings after update | ||
| */ | ||
| protected List<Settings> addExtraClusterBootstrapSettings(List<Settings> allNodesSettings) { |
There was a problem hiding this comment.
I think this is never overridden, so can be inlined.
reset bootstrapMasterNodeId after bootstrap
# 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
force-pushed
the
zen2_tests_bootstrap
branch
from
66982b5 to
738cf9f
Compare
|
@DaveCTurner thanks for your review! |
|
I've missed that |
* 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) ...
3 participants
Currently, there are a few tests that use autoMinMasterNodes=false and
hence override
addExtraClusterBootstrapSettings, mostly this is 10-30lines of codes that are copy-pasted from class to class.
This PR introduces
bootstrapMasterNodeWithSpecifiedIdwhich issuitable for all classes and copy-paste could be removed.
Removing code is always a good thing!