Fix incorrect mutex used in QueryPhaseResultConsumer #116171
Conversation
Everything synchronizes on PendingMerges, this was just a typo. closes elastic#115716
original-brownbear
added
>non-issue
v8.7.0
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Looking at the PR that originally added the sync, #113355 Seems like it is only in v9? Not sure why this has a Also, I don't understand why we would |
|
Thanks for taking a look Ben!
sorry that was just me being stupid, just spotted the first mistake and got too happy that it made the failures go away ... fixed both spots now.
My bad yet again, should back port the other PR too. Will fix this up after. |
There was a problem hiding this comment.
All locking is now done on PendingMerges & MergeTask object instances. This seems better.
I do not fully know if this will fix the overall issues. But its an obvious bug in the previous refactoring. So let's first fix this. If this doesn't work, then I suggest we revert the refactoring @original-brownbear #113355
this code is tricky :)
|
Thanks Ben! I think this code just shouldn't be so tricky. I'm gonna follow-up here with some serious simplification now. All this does is queue and once we queued enough, merge + some checks on the stuff queued. There's no need for this to be hard :) |
No need for a separate PendingMerges nested class, this thing has a lot of state, lets accept that (for now) and keep it simple. Having a redundant nested class already caused a bug in elastic#116171 and just adds overhead both in runtime and when reading the code.
|
Actually @benwtrent turns out the remaining issue here is the result of #113230 (also mine though, sorry :/ ...) fixing it. |
No need for a separate PendingMerges nested class, this thing has a lot of state, lets accept that (for now) and keep it simple. Having a redundant nested class already caused a bug in #116171 and just adds overhead both in runtime and when reading the code.
Everything synchronizes on PendingMerges, this was just a typo. closes #115716
No need for a separate PendingMerges nested class, this thing has a lot of state, lets accept that (for now) and keep it simple. Having a redundant nested class already caused a bug in #116171 and just adds overhead both in runtime and when reading the code.
No need for a separate PendingMerges nested class, this thing has a lot of state, lets accept that (for now) and keep it simple. Having a redundant nested class already caused a bug in elastic#116171 and just adds overhead both in runtime and when reading the code.
4 participants
Everything synchronizes on PendingMerges, this was just a typo.
Non-issue since this hasn't been released yet. This likely fixes a bunch of other tests, I'll open a separate PR for those after going through them 1 by 1.
closes #115716