Deduplicate allocation stats calls #123246
Conversation
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work.
DaveCTurner
added
>bug
:Distributed Coordination/Allocation
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
original-brownbear
left a comment
original-brownbear
left a comment
There was a problem hiding this comment.
Looks just fine except I think we need to change where we fork still?
| actionFilters, | ||
| TransportGetAllocationStatsAction.Request::new, | ||
| TransportGetAllocationStatsAction.Response::new, | ||
| threadPool.executor(ThreadPool.Names.MANAGEMENT) |
There was a problem hiding this comment.
You wouldn't want to fork anymore with the deduplicator would you? Only fork in case you actually do the computation?
There was a problem hiding this comment.
Technically yes that's right of course. I'm always in two minds about adding more non-forking actions: they increase the risk of serious pain in future (for nontechnical reasons) at the cost of a little more latency right now. I'll let you have this one without prejudice tho 😉
There was a problem hiding this comment.
You'd think forking lowers the risk of pain the future, but under load already are at the other end of this.
Forking is far more expensive than just the fork in the real world. You also need to account for the fact that you'll allocate a buffer off of the channel's thread and release that buffer which comes with contention very quickly unfortunately. Just to illustrate this a little :)
| final var startBarrier = new CyclicBarrier(threads.length); | ||
| for (int i = 0; i < threads.length; i++) { | ||
| threads[i] = new Thread(() -> { | ||
| safeAwait(startBarrier); |
There was a problem hiding this comment.
NIT: could use org.elasticsearch.test.ESTestCase#startInParallel here ? :)
DaveCTurner
added
auto-merge-without-approval
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work.
💔 Backport failed
You can use sqren/backport to manually backport by running |
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work. Backport of elastic#123246 to `8.x`
|
8.x-and-earlier backports are #123267 |
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work. Backport of #123246 to `8.x`
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work. Backport of elastic#123246 to `8.x`
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work. Backport of elastic#123246 to `8.x`
These things can be quite expensive and there's no need to recompute them in parallel across all management threads as done today. This commit adds a deduplicator to avoid redundant work. Backport of elastic#123246 to `8.x`
3 participants
These things can be quite expensive and there's no need to recompute
them in parallel across all management threads as done today. This
commit adds a deduplicator to avoid redundant work.