Fix ThreadPoolMergeSchedulerStressTestIT testMergingFallsBehindAndThenCatchesUp #125956
Conversation
albertzaharovits
added
>test
elasticsearchmachine
added
Team:Distributed Indexing
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
| void allowAllMerging() { | ||
| // even when indexing is done, queued and backlogged merges can themselves trigger further merging | ||
| // don't let this test be bothered by that, and simply let all merging run unhindered | ||
| runMergeSemaphore.release(Integer.MAX_VALUE - initialRunMergesCount); |
There was a problem hiding this comment.
The semaphore was also released when excess merges were enqueued. This is a dynamic process. I preferred I not fudge an upper limit value, or put Integer.MAX_VALUE / 2 ...
| // await all merging to catch up | ||
| assertBusy(() -> { | ||
| // unblock merge threads | ||
| testEnginePlugin.allowAllMerging(); |
There was a problem hiding this comment.
Dynamically release merge permits until everything's done.
| runMergeSemaphore.release(Integer.MAX_VALUE - initialRunMergesCount); | ||
| if (runMergeSemaphore.availablePermits() < 10_000) { | ||
| runMergeSemaphore.release(10_000); | ||
| } |
There was a problem hiding this comment.
I am not quite sure what we are doing here. Isn't the maximum number of permits we can release = initialRunMergesCount - runMergeSemaphore.availablePermits() ?
There was a problem hiding this comment.
We need to release as many permits as there are merges, but we don't know how many of those are going to be generated (depends on many parameters). And the fact that there is some "background" merging while merging is otherwise mostly stopped, complicates computations.
| // even when indexing is done, queued and backlogged merges can themselves trigger further merging | ||
| // don't let this test be bothered by that, and simply let all merging run unhindered | ||
| runMergeSemaphore.release(Integer.MAX_VALUE - initialRunMergesCount); | ||
| if (runMergeSemaphore.availablePermits() < 10_000) { |
There was a problem hiding this comment.
How can it have more than 10K available already? Should we instead assert it has not?
There was a problem hiding this comment.
It can't have more than 10K, or Integer.MAX_VALUE / 2, 99999, etc...
It's just strange to fudge any sort of non-obvious value, but on the other hand, the shenanigans required to avoid that are not worth it. I'll fudge a value, and drop a comment.
…nCatchesUp (elastic#125956) We don't know how many semaphore merge permits we need to release, or how many are already released. Fixes elastic#125744
…nCatchesUp (elastic#125956) We don't know how many semaphore merge permits we need to release, or how many are already released. Fixes elastic#125744
4 participants
We don't know how many semaphore merge permits we need to release, or how many are already released.
Fixes #125744