Concurrent thread access to shared doc values #99007
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Hi @salvatore-campagna, I've created a changelog YAML for you. |
|
By the way, I am seeing other errors in the test I was able to reproduce it on 8.9 so it is something that has been probably always there. Note that to silent the errors on this issue you can add the following annotation at the top of the class: |
I agree that is probably something that has always been there. |
|
I think there's an issue with |
|
|
||
| public void testDateHistogramByTsid() { | ||
| final TimeSeriesAggregationBuilder timeSeries = new TimeSeriesAggregationBuilder("ts").subAggregation( | ||
| new DateHistogramAggregationBuilder("date_histogram").field("@timestamp").calendarInterval(DateHistogramInterval.MINUTE) |
There was a problem hiding this comment.
Change the interval to HOUR to avoid exceeding bucket limit?
This makes sense to me...indeed sometimes the test was failing with which actually confirms the doc values being shared and used by multiple threads. Thanks @kkrik-es for looking at this. |
|
I pushed the changes suggested by Kostas but I still see some failures. |
|
The fix from Kostas does not address the issue of accessing the doc values from different threads. That's a different beast. Are you getting a different exception? |
|
I think I know how to fix the issue. My proposal is that you add |
|
Or you can fix it here: |
| @@ -105,16 +105,23 @@ public ScoreMode scoreMode() { | |||
| private class CompetitiveIterator extends DocIdSetIterator { | |||
|
|
|||
| private final BitArray visitedOrds; | |||
| private final SortedSetDocValues values; | |||
There was a problem hiding this comment.
Use a different name here to avoid confusion?
There was a problem hiding this comment.
I remove this...it is not needed (see following commit)
| @@ -211,6 +218,7 @@ public LeafBucketCollector getLeafCollector(AggregationExecutionContext aggCtx, | |||
| if (maxOrd <= MAX_FIELD_CARDINALITY_FOR_DYNAMIC_PRUNING || numNonVisitedOrds <= MAX_TERMS_FOR_DYNAMIC_PRUNING) { | |||
| dynamicPruningAttempts++; | |||
| return new LeafBucketCollector() { | |||
| final SortedSetDocValues docValues = valuesSource.globalOrdinalsValues(aggCtx.getLeafReaderContext()); | |||
There was a problem hiding this comment.
Does this work:
final SortedSetDocValues docValues = values;
There was a problem hiding this comment.
Agree, it makes no sense if you are calling the same above
There was a problem hiding this comment.
I wanted to remove values completely...but it is used by postCollection that is why I was doing that.
I will restore
docValues = values
to avoid calling methods unnecessarily.
| @@ -267,6 +275,8 @@ public CompetitiveIterator competitiveIterator() { | |||
|
|
|||
| bruteForce++; | |||
| return new LeafBucketCollector() { | |||
| final SortedSetDocValues docValues = valuesSource.globalOrdinalsValues(aggCtx.getLeafReaderContext()); | |||
|
I am running the test (testCardinalityByTsid) until failure and after more than 1000 runs I don't see any issue. |
Thanks @iverase ...if I understand correctly the result of doing this is that
So |
|
I think this change should be backported to 8.10.x if possible as there are lingering issues in that line. |
|
This PR should not have the label backport, it is the backport PR that should have it |
|
Hi @salvatore-campagna, I've created a changelog YAML for you. |
|
Error is legit, we are calling postCollection twice for downsampling. We need to remove the following line: And there are other cases in |
So the empty value I see is a result of the doc values "iterator" reaching the end of the stream and being used again by the second invocation? |
|
@iverase if I remove it from |
You should only remove it when using the time series searcher |
💚 Backport successful
|
The doc values in the `GlobalOrdCardinalityAggregator` are shared among multiple search threads, `search` and `search_worker`. The search thread also runs the aggregation phase. When an executor is used the 'search' thread is running `postCollection`, which uses doc values, while other methods are executed by the `search_worker` thread, using doc values too. As a result, doc values are concurrently accessed by different threads. Using doc values concurrently from multiple threads is not correct since multiple threads end up updating the doc values state. This breaks access to doc values resulting in different issue depending on how threads end up being scheduled (prematurely exhausting doc values, accessing incorrect documents as a result of trying to access docIds not in the thread owned leaf/segment,...). The solution here is to: 1. make sure we executed `postCollection in the same thread as other methods, which is `search` or `search_worker`. 2. make sure we do not call `postCollection` in case the `TimeSeriesIndexSearcher` is used. In that case `postCollection` is called by `TimeSeriesIndexSearcher`.
The doc values in the `GlobalOrdCardinalityAggregator` are shared among multiple search threads, `search` and `search_worker`. The search thread also runs the aggregation phase. When an executor is used the 'search' thread is running `postCollection`, which uses doc values, while other methods are executed by the `search_worker` thread, using doc values too. As a result, doc values are concurrently accessed by different threads. Using doc values concurrently from multiple threads is not correct since multiple threads end up updating the doc values state. This breaks access to doc values resulting in different issue depending on how threads end up being scheduled (prematurely exhausting doc values, accessing incorrect documents as a result of trying to access docIds not in the thread owned leaf/segment,...). The solution here is to: 1. make sure we executed `postCollection in the same thread as other methods, which is `search` or `search_worker`. 2. make sure we do not call `postCollection` in case the `TimeSeriesIndexSearcher` is used. In that case `postCollection` is called by `TimeSeriesIndexSearcher`.
When trying to run a cardinality aggregation nested inside a
time series aggregation test called
testCardinalityByTsid(sometimes) fails with the following stack traces (plural here
is not a mistake, the test appears to fail with different issues).
It looks like something is wrong when accessing dimension fields
doc values.
My idea is that something is wrong with ordinals but can't figure out
if that is the case.
Usually I see one of the following two assertions failing:
which means in
GlobalOrdCardinalityAggregatorwe tryto fetch incorrect target document when calling
advanceExactNote also that this branch is exercised only if the cardinality aggregation is
not a top level aggregation. I tried to reproduce the issue with the cardinality
aggregation nested inside a terms aggregation but didn't see any issue.
For this reason I believe something might be wrong when using parent (time
series aggregator) ordinals.
Also worth noting is that sometimes the test fails with other issues. I executed the
test a certain number of times to see it failing. Usually it takes less than 10 executions
to see a failure.
This is another different stack trace