Add cancellation support in TransportGetAllocationStatsAction
#127371
Conversation
JeremyDahlgren
added
>feature
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Hi @JeremyDahlgren, I've created a changelog YAML for you. |
JeremyDahlgren
force-pushed
the
fix/123248
branch
from
040b400 to
14918db
Compare
JeremyDahlgren
force-pushed
the
fix/123248
branch
from
14918db to
8d6f7cc
Compare
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
My feeling is that there has to be a simpler way to achieve what we want than adding all this bare-handed locking etc. I would have expected something based on ref-counting would be neater: each waiting listener should hold a ref, releasing it on cancellation, and an ongoing computation stops early if the number of waiting refs drops to zero.
...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java
Outdated
Show resolved
Hide resolved
...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java
Outdated
Show resolved
Hide resolved
...ava/org/elasticsearch/action/admin/cluster/allocation/TransportGetAllocationStatsAction.java
Outdated
Show resolved
Hide resolved
I applied your suggested change to |
|
@DaveCTurner - when you have some time could you take a pass at reviewing this new version that uses |
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
One request for a missing test, otherwise just tiny nits. Production code looks good.
| private volatile long ttlMillis; | ||
| private final ThreadPool threadPool; | ||
| private final AtomicReference<CachedAllocationStats> cachedStats; | ||
| private final ExecutorService executorService; |
There was a problem hiding this comment.
nit: could this be an Executor? No need for it to be AutoCloseable (it's a disaster if you try and close one of the threadpool's executors)
| routingAllocation.metadata(), | ||
| routingAllocation.routingNodes(), | ||
| routingAllocation.clusterInfo(), | ||
| () -> {}, |
There was a problem hiding this comment.
Could we make this into a static constant so that it has a name, something like NEVER_CANCELLED perhaps? Otherwise it leaves the reader wondering what this lambda is for (and also saves allocating a fresh lambda each time it's called, although the compiler might be clever enough to skip that anyway)
| /** | ||
| * Sets the currently cached item reference to {@code null}, which will result in a {@code refresh()} on the next {@code get()} call. | ||
| */ | ||
| protected void clearCurrentCachedItem() { |
There was a problem hiding this comment.
This isn't covered by the CancellableSingleObjectCache test suite yet. Also should this be final? We don't want subclasses overriding it.
| } | ||
|
|
||
| private void verifyAllocationStatsServiceNumCallsEqualTo(int numCalls) { | ||
| verify(allocationStatsService, times(numCalls)).stats(argThat(r -> true)); |
There was a problem hiding this comment.
Is this the same thing?
| verify(allocationStatsService, times(numCalls)).stats(argThat(r -> true)); | |
| verify(allocationStatsService, times(numCalls)).stats(any()); |
There was a problem hiding this comment.
Yes, I applied this in the other uses as well.
3 participants
Replaces the use of a
SingleResultDeduplicatorby refactoring the cache as a subclass ofCancellableSingleObjectCache. Refactored theAllocationStatsServiceandNodeAllocationStatsAndWeightsCalculatorto accept theRunnableused to test for cancellation.Closes #123248