Remove Needless Sleeps on Node Configuration Changes in Internal Cluster Tests #76884
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ter Tests I noticed this recently when trying to reproduce a test failure. We're doing a lot of sleeping when validating that the cluster formed if that process is slow randomly (which it tends to be due to disk interaction on node starts and such.). By reusing the approach for waiting on a cluster state we rarely if ever need to get into the busy assert loop and remove all these sleeps, shaving of a few seconds here and there from running internal cluster tests.
original-brownbear
added
>test
Collaborator
|
Pinging @elastic/es-delivery (Team:Delivery) |
Contributor
Author
|
Jenkins run elasticsearch-ci/packaging-tests-windows-sample (unrelated windows worker randomness) |
DaveCTurner
approved these changes
Aug 24, 2021
Contributor
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, left a couple of suggestions
test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void onTimeout(TimeValue timeout) { | ||
| future.onFailure(new TimeoutException()); |
Contributor
There was a problem hiding this comment.
Suggested change
| future.onFailure(new TimeoutException()); | |
| assert false : "onTimeout called with no timeout set"; |
Contributor
Author
There was a problem hiding this comment.
This isn't quite correct I think, the default timeout on the observer is 60s isn't it?
Contributor
There was a problem hiding this comment.
Oh right so it is. I saw lots of @Nullable things, missed that one place. Still, we don't want the observer to timeout do we? We should pass null to avoid enqueueing the timeout task.
Contributor
Author
There was a problem hiding this comment.
I guess it doesn't matter much since we only wait on the future for 30s so we won't ever see those 60s elapse. But you're right, much cleaner to not enqueue any timeout :)
Contributor
Author
|
Thanks David! |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Aug 25, 2021
…ter Tests (elastic#76884) I noticed this recently when trying to reproduce a test failure. We're doing a lot of sleeping when validating that the cluster formed if that process is slow randomly (which it tends to be due to disk interaction on node starts and such.). By reusing the approach for waiting on a cluster state we rarely if ever need to get into the busy assert loop and remove all these sleeps, shaving of a few seconds here and there from running internal cluster tests.
original-brownbear
added a commit
that referenced
this pull request
Aug 25, 2021
…ter Tests (#76884) (#76908) I noticed this recently when trying to reproduce a test failure. We're doing a lot of sleeping when validating that the cluster formed if that process is slow randomly (which it tends to be due to disk interaction on node starts and such.). By reusing the approach for waiting on a cluster state we rarely if ever need to get into the busy assert loop and remove all these sleeps, shaving of a few seconds here and there from running internal cluster tests.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Mar 29, 2022
Since elastic#76884 in `InternalTestCluster#validateClusterFormed` we wait for a correctly-sized cluster state to be applied before entering the `assertBusy()` loop to wait for the cluster state to be exactly right everywhere. Today we do this by injecting a cluster state observer into one of the nodes which waits for a cluster state containing a master and the right number of nodes. With this commit we move to using the cluster health API which can do the same thing. By this point any extra nodes have stopped, but there might still be a stale join request for one of those nodes in the master's queue. This commit addresses this by also waiting for the master queue to be empty. Closes elastic#81830
DaveCTurner
added a commit
that referenced
this pull request
May 6, 2022
Since #76884 in `InternalTestCluster#validateClusterFormed` we wait for a correctly-sized cluster state to be applied before entering the `assertBusy()` loop to wait for the cluster state to be exactly right everywhere. Today we do this by injecting a cluster state observer into one of the nodes which waits for a cluster state containing a master and the right number of nodes. With this commit we move to using the cluster health API which can do the same thing. By this point any extra nodes have stopped, but there might still be a stale join request for one of those nodes in the master's queue. This commit addresses this by also waiting for the master queue to be empty. Closes #81830
DaveCTurner
added a commit
that referenced
this pull request
May 6, 2022
Since #76884 in `InternalTestCluster#validateClusterFormed` we wait for a correctly-sized cluster state to be applied before entering the `assertBusy()` loop to wait for the cluster state to be exactly right everywhere. Today we do this by injecting a cluster state observer into one of the nodes which waits for a cluster state containing a master and the right number of nodes. With this commit we move to using the cluster health API which can do the same thing. By this point any extra nodes have stopped, but there might still be a stale join request for one of those nodes in the master's queue. This commit addresses this by also waiting for the master queue to be empty. Closes #81830
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I randomly noticed this recently when trying to reproduce a test failure. We're doing a lot of sleeping
when validating that the cluster formed if that process is slow randomly (which it tends to be
due to disk interaction on node starts and such.). By reusing the approach for waiting on a
cluster state we rarely if ever need to get into the busy assert loop and remove all these sleeps,
shaving of a few seconds here and there from running internal cluster tests (at the cost of minimal
added complexity).