Fix assertion error on multiple node search reduce errors #131085
Conversation
cbuescher
added
>test
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
I ran into this issue while working on #129445 where another test in this suite fails for probably other reasons ("testSortMixedFieldTypes"). The test provokes IllegalArgumentExceptions when sorting on two indices with incompatible sort types. Without the change in I'm not sure preventing propagating any subsequent phase failure in AbstractSearchAsyncAction like suggested here is the right way to solve this, also I'm not really sure what big of an issue calling the ActionListener twice is in practice but the test assertion seems to be there for a reason. Also this might still be related to the weird errors we get in #129445 since that issue also seems to be related to search exception handling. |
benchaplin
left a comment
benchaplin
left a comment
There was a problem hiding this comment.
I agree it seems wrong to raise multiple phase failures for multiple data node failures.
I'm hesitant about this change, is it possible we're relying on multiple runs of raisePhaseFailure / listener.onFailure(exception) here, one per node search exception? Would love to hear more of your thoughts on that.
| * @param cause the cause of the phase failure | ||
| */ | ||
| public void onPhaseFailure(String phase, String msg, Throwable cause) { | ||
| if (phaseFailureEncountered.compareAndSet(false, true) == false) { |
There was a problem hiding this comment.
Maybe this is what we want, but this change would be lighter weight if this check was moved to the caller in SearchQueryThenFetchAsyncAction.
benchaplin
left a comment
benchaplin
left a comment
There was a problem hiding this comment.
I've looked into this more after reviewing a similar exception in TransportSearchIT due to batched query execution. I'm now convinced this fix is correct - I think it makes the most sense to protect against calling raisePhaseFailure twice.
|
@javanna @benchaplin fyi just linked #134151 which looks like a similar failure. |
benchaplin
dismissed
their stale review
Holding off as we continue to discuss...
When receiving exceptions from more than one node on data node search reduction, SearchQueryThenFetchAsyncAction will raise more than one phase failure. This can lead to calling the listener in AbstractSearchAsyncAction more than once, which in turn in tests trips an assertion in ActionListener#assertFirstRun.
cbuescher
force-pushed
the
fix-multiple-reduce-errors-assertion
branch
from
576cee9 to
72999c2
Compare
|
@benchaplin I rebased and updated this fix as suggested in #134151, also verified that the reproducing test case from that issue passes now. I'm not sure if it is worth adding that repro since its pretty specific for aggs. Also I was considering moving the test added here to some other location than FieldSortIT since its main focus now should be testing the failure handling. With your recent changes in that area maybe you can suggest an alternative location? |
|
@benchaplin after reading your comment on #134151 (comment) I think this PR either needs a follow up or should be closed in favor of another approach you sketched out in that comment. I'll take a look later how I would approach freeing contexts of in-flight requests as you suggested as a modification for this PR but will probably time-box that if it gets too involved. |
|
Agreed @cbuescher, this change might be part of a solution but definitely not the whole thing. Don't sweat looking into it to much, I've been doing the same already :). Thanks for staying in the loop on this stuff with me! |
|
@benchaplin quickly checking if I should close this in favor of #136889, wdyt? |
|
Yep @cbuescher let's close this. |
3 participants
When receiving exceptions from more than one node on data node search reduction, SearchQueryThenFetchAsyncAction will raise more than one phase failure. This can lead to calling the listener in AbstractSearchAsyncAction more than once, which in turn in tests trips an assertion in ActionListener#assertFirstRun.