Nonblocking master service#94325
Conversation
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.
844ad98 to
b0a4aa8
Compare
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.
|
Pinging @elastic/es-distributed (Team:Distributed) |
DaveCTurner
left a comment
There was a problem hiding this comment.
tip: ignoring whitespace will make this easier to review
henningandersen
left a comment
There was a problem hiding this comment.
I think this looks good, but have a question/comment that you can maybe clarify quickly?
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. |
| 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.
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
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
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.