Send max of two types of max queue latency to ClusterInfo #132675
Merged
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
The TransportNodeUsageStatsForThreadPoolsAction now takes the max latency of any task currently queued in the write thread pool queue AND the previously collected max queue latency of any task dequeued since the last call. This covers the possibility that queue times can rise greatly before being reflected in execution: imagine all the write threads are stalled. Adds additional IT testing to exercise both forms of queue latency, a followup for ES-12316.
DiannaHohensee
added
>enhancement
:Distributed Coordination/Allocation
Collaborator
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
Collaborator
|
Hi @DiannaHohensee, I've created a changelog YAML for you. |
DiannaHohensee
commented
Aug 11, 2025
| * that is returned, which waits for total-write-threads + 1 callers. The caller can release the tasks by calling | ||
| * {@code barrier.await()} or interrupt them with {@code barrier.reset()}. | ||
| */ | ||
| public CyclicBarrier blockDataNodeIndexing(String dataNodeName) { |
Contributor
Author
There was a problem hiding this comment.
I stole this from some serverless testing.
DiannaHohensee
commented
Aug 11, 2025
| ); | ||
| } | ||
|
|
||
| public static void waitForTimeToElapse(long elapsedMillis) throws InterruptedException { |
Contributor
Author
There was a problem hiding this comment.
Pulling this from another test file, so it can be reused.
Contributor
There was a problem hiding this comment.
I think we do not need to reuse this - so prefer to keep it local instead. Ideally we'd not have such unqualified waits, but we can look at that separately.
DiannaHohensee
changed the title
Contributor
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
Left a number of comments, otherwise looks good.
I wonder if we really need this in a first version, but now it is here, it is fine to get in.
.../org/elasticsearch/common/util/concurrent/TaskExecutionTimeTrackingEsThreadPoolExecutor.java
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| public static void waitForTimeToElapse(long elapsedMillis) throws InterruptedException { |
Contributor
There was a problem hiding this comment.
I think we do not need to reuse this - so prefer to keep it local instead. Ideally we'd not have such unqualified waits, but we can look at that separately.
…e write thread pool: eliminate replicas requiring sync
DiannaHohensee
commented
Aug 14, 2025
Contributor
Author
DiannaHohensee
left a comment
DiannaHohensee
left a comment
There was a problem hiding this comment.
Thanks for the review, I've addressed the feedback.
I also realized that the peek() method was returning Nanos, not Millis, so I fixed that (a23c716).
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
|
|
||
| public static void waitForTimeToElapse(long elapsedMillis) throws InterruptedException { |
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java
Outdated
Show resolved
Hide resolved
.../org/elasticsearch/common/util/concurrent/TaskExecutionTimeTrackingEsThreadPoolExecutor.java
Show resolved
Hide resolved
…must wait for 1ms, not 1ns, to see a change in queue latency
DiannaHohensee
added
the
auto-merge-without-approval
joshua-adams-1
pushed a commit
to joshua-adams-1/elasticsearch
that referenced
this pull request
Aug 15, 2025
…2675) The TransportNodeUsageStatsForThreadPoolsAction now takes the max latency of any task currently queued in the write thread pool queue AND the previously collected max queue latency of any task dequeued since the last call. This covers the possibility that queue times can rise greatly before being reflected in execution: imagine all the write threads are stalled or have long running tasks. This action feeds a max queue latency stat to the ClusterInfo. Follow up from ES-12233. Adds additional IT testing to exercise both forms of queue latency, a followup for ES-12316. ------------- Completing the follow up testing [Henning requested previously](https://github.com/elastic/elasticsearch/pull/131480/files#r2243610807).
This was referenced Sep 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
The TransportNodeUsageStatsForThreadPoolsAction now
takes the max latency of any task currently queued
in the write thread pool queue AND the previously
collected max queue latency of any task dequeued
since the last call. This covers the possibility
that queue times can rise greatly before being
reflected in execution: imagine all the write
threads are stalled or have long running tasks.
This action feeds a max queue latency stat to
the ClusterInfo. Follow up from ES-12233.
Adds additional IT testing to exercise both
forms of queue latency, a followup for ES-12316.
Completing the follow up testing Henning requested previously.