Remove dependency on cluster state API in SpecificMasterNodesIT
#127213
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2036,20 +2036,46 @@ public String getMasterName() { | |
| * in the viaNode parameter. If viaNode isn't specified a random node will be picked to the send the request to. | ||
| */ | ||
| public String getMasterName(@Nullable String viaNode) { | ||
| viaNode = viaNode != null ? viaNode : getRandomNodeName(true); | ||
| try { | ||
| ClusterServiceUtils.awaitClusterState(logger, state -> state.nodes().getMasterNode() != null, clusterService(viaNode)); | ||
| final ClusterState state = client(viaNode).admin().cluster().prepareState(TEST_REQUEST_TIMEOUT).setLocal(true).get().getState(); | ||
| return state.nodes().getMasterNode().getName(); | ||
| } catch (Exception e) { | ||
| logger.warn("Can't fetch cluster state", e); | ||
| throw new RuntimeException("Can't get master node " + e.getMessage(), e); | ||
| } | ||
|
Comment on lines
2040
to
2047
There was a problem hiding this comment. I still worry this reads the state twice and might see no master the second time due to election jitter. We could cheat and remember the master node seen in the state on which we're awaiting (also saves the exception handling): final var masterNameListener = new SubscribableListener<String>();
return safeAwait(ClusterServiceUtils.addTemporaryStateListener(clusterService(viaNode), cs -> {
Optional.ofNullable(cs.nodes().getMasterNode()).ifPresent(masterNode -> masterNameListener.onResponse(masterNode.getName()));
return masterNameListener.isDone();
}).andThen(masterNameListener::addListener));There was a problem hiding this comment. I thought about that too, but I figured "election jitter" would likely disrupt the test anyway. Tests should be designed to be deterministic/consistent, but there's only so much outside influence tests can account for (e.g. election jitter or CI blips). I'm a little hesitant to "cheat" here. I seem to recall someone saying
There was a problem hiding this comment. Heh yeah I thought I'd said that at some point :) On reflection I think you're right, tests should not be calling this if the master isn't stable. |
||
| } | ||
|
|
||
| public String getNonMasterNodeName() { | ||
| NodeAndClient randomNodeAndClient = getRandomNodeAndClient(new NodeNamePredicate(getMasterName()).negate()); | ||
| if (randomNodeAndClient != null) { | ||
| return randomNodeAndClient.getName(); | ||
| } | ||
| throw new AssertionError("No non-master node found"); | ||
| } | ||
|
|
||
| /** | ||
| * @return the name of a random node in a cluster | ||
| */ | ||
| public String getRandomNodeName() { | ||
| return getRandomNodeName(false); | ||
| } | ||
|
|
||
| /** | ||
| * @param startNewNode whether we should start a new node if there are no nodes in the cluster | ||
| * @return the name of a random node in a cluster | ||
| */ | ||
| public String getRandomNodeName(boolean startNewNode) { | ||
| String node = getNodeNameThat(Predicates.always()); | ||
| ensureOpen(); | ||
| if (node == null) { | ||
| synchronized (this) { | ||
| return getOrBuildRandomNode().getName(); | ||
|
||
| } | ||
| } else { | ||
| return node; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
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 added these methods in
ESIntegTestCasebecause I see other use cases for these methods (which I'll get to in follow-up PRs). I'm a little bit on the fence about which class they should live in, though. I would actually like to avoid adding them in this class, as this class is too big already. MaybeInternalTestCluster? Or an entirely new class?