Nonblocking master service #94325
Merged
Nonblocking master service #94325
Conversation
This file contains 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
DaveCTurner
added
>non-issue
:Distributed Coordination/Cluster Coordination
The changes introduced in elastic#92021 mean that there is no need for the master service to block its thread while waiting for each publication to complete. This commit removes the now-unnecessary blocking. This commit also removes the now-unnecessary fake blocking in the `FakeThreadPoolMasterService` used in tests, bringing the implementation covered by tests closer to the production implementation.
DaveCTurner
force-pushed
the
2023-03-06-nonblocking-master-service
branch
from
844ad98 to
b0a4aa8
Compare
DaveCTurner
commented
Mar 6, 2023
| @@ -2369,7 +2377,7 @@ public void taskSucceeded(ClusterStateUpdateTask clusterStateTaskListener, Void | |||
| public ClusterState execute(ClusterState currentState) { | |||
| assertEquals(priority, prioritiesQueue.poll()); | |||
| assertEquals(priority, priority()); | |||
| return currentState; | |||
| return randomBoolean() ? currentState : ClusterState.builder(currentState).build(); | |||
There was a problem hiding this comment.
We're using a single-threaded executor here, so by sometimes triggering a publication and sometimes forking the publication handling onto a separate thread we demonstrate that there is no longer any blocking involved in the publication process.
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
DaveCTurner
commented
Mar 6, 2023
server/src/main/java/org/elasticsearch/cluster/service/MasterService.java
Show resolved
Hide resolved
|
Bah, ok, we're not always on the master thread where I thought we were (because rejections might leave us on the applier thread). I'll remove that comment for now and follow up later. |
henningandersen
approved these changes
Mar 30, 2023
| threadPool.executor(randomFrom(ThreadPool.Names.SAME, ThreadPool.Names.GENERIC)).execute(() -> { | ||
| final var threadName = Thread.currentThread().getName(); | ||
| try { | ||
| Thread.currentThread().setName("[" + MasterService.MASTER_UPDATE_THREAD_NAME + "]"); |
There was a problem hiding this comment.
This seems unnecessary now that we use the ThreadedActionListener. I think we should remove this line (and the try-finally), that would validate that we can call the publish listener from any thread.
There was a problem hiding this comment.
Thanks, yes - see 464358c.
DaveCTurner
added
the
auto-merge-without-approval
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Mar 30, 2023
These tests submit tasks to the master service and wait for the state to be applied, but not for the publication to complete, and without that wait they may clean up too soon. This commit makes them wait for the master service to finish its work before cleaning up. Relates elastic#94325 Closes elastic#94900
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 5, 2023
In elastic#94325 we introduced another forking step when submitting a publication, so we must extend the timeout in this test (and `DEFAULT_CLUSTER_STATE_UPDATE_DELAY`) by `DEFAULT_DELAY_VARIABILITY`. Closes elastic#94905
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The changes introduced in #92021 mean that there is no need for the master service to block its thread while waiting for each publication to complete. This commit removes the now-unnecessary blocking.
This commit also removes the now-unnecessary fake blocking in the
FakeThreadPoolMasterServiceused in tests, bringing the implementation covered by tests closer to the production implementation.