Aggs: Fix Filters agg cancellation flaky test #130810
Conversation
ivancea
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| assertThat(searchRequestFuture.isCancelled(), equalTo(false)); | ||
| assertThat(searchRequestFuture.isDone(), equalTo(false)); |
There was a problem hiding this comment.
The message looks better this way IMO, no functional change here
| assertBusy(() -> { | ||
| assertTrue(searchRequestFuture.isDone()); | ||
| assertThat(getSearchTasks(), empty()); | ||
| assertThat("Search request didn't finish", searchRequestFuture.isDone(), equalTo(true)); |
There was a problem hiding this comment.
Isn't there an assertTrue that also takes a String?
There was a problem hiding this comment.
Scratch that, I see now that you used it to get the hamcrest error message, and apparently assertTrue(msg, bool) is from junit, not hamcrest
There was a problem hiding this comment.
Yeah. The message with assertTrue/False was just "AssertionError: null", while the equalTo(true) was like "false != true". Which isn't much better, but at least tells you something. Anyway, with the message they should be similar, but I changed them anyway XD
| return PauseScriptPlugin.class; | ||
| @Override | ||
| public Settings nodeSettings(int nodeOrdinal, Settings otherSettings) { | ||
| return Settings.builder().put(super.nodeSettings(nodeOrdinal, otherSettings)).put("thread_pool.search.size", 4).build(); |
There was a problem hiding this comment.
Out of curiosity, why 4?
There was a problem hiding this comment.
Good question! That was the core of the issue:
Some context first:
- The
CancellableBulkScorerwe use to break the execution is called per search thread in the query. - It breaks the "for each doc" into blocks of 4096 docs (x2 every iteration), and checks for cancellation between blocks
- The test uses 100.000 docs.
- The test adds around 99000 permits to a semaphore, so only 99000 docs can be processed before the threads getting blocked (And the test failing)
- Which is what the test does: It expect cancelled queries to not reach that many processed docs, and break earlier.
Now, if there are 25 threads, it would consume up to 25*4096 = 102400 docs (before checking for cancellation), which would be more than the semaphore permits, and threads would get blocked. Which is what happened sometimes.
To the specific question: 4 is just a small number that shouldn't trigger this case by a great margin.
There was a problem hiding this comment.
Suggestion: Either add a short version of this explanation as a comment, or replace 4 with some computation based on the total number of documents.
There was a problem hiding this comment.
Added a test javadoc explaining what it does, and how it does it (Including those magic constants)
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
Fixes elastic#130770 Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck). The test was added in elastic#130452
4 participants
Fixes #130770
Both a bigger thread pool and not draining the semaphore permits were leading to failing tests sometimes because of blocked threads (Too many threads searching would end up draining all permits in parallel, and getting stuck).
The test was added in #130452