TSDB: Add time series aggs cancellation#83492
Conversation
Adds support for low-level cancelling time-series based aggregations before they reach the reduce phase. Relates to elastic#74660
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
|
||
| @Override | ||
| public int nextDoc() throws IOException { | ||
| checkCancelled.run(); |
There was a problem hiding this comment.
Do we want to run cancellation checks on every single move of the iterator or would it be better to do some sampling here? This could get very expensive.
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
Looks good in general, I left a couple of comments around the scorer and weight implementations.
| return new Weight(weight.getQuery()) { | ||
| @Override | ||
| public Explanation explain(LeafReaderContext context, int doc) throws IOException { | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
Delegate to weight.explain(context, doc) here?
|
|
||
| @Override | ||
| public boolean isCacheable(LeafReaderContext ctx) { | ||
| throw new UnsupportedOperationException(); |
| * The main purpose of this class is to allow cancellation of search requests. Note that this class doesn't wrap bulk scorer, for that | ||
| * use {@link CancellableBulkScorer} instead. | ||
| */ | ||
| public class CancellableScorer extends Scorer { |
There was a problem hiding this comment.
You could extend FilterScorer here which would simplify things a bit
There was a problem hiding this comment.
Yeah, that was my first attempt, but it wasn't possible due to some methods being final. But it is a really sensible suggestion, so I think I should probably add a comment explaining all this.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I think it'd be kind of nice to have a unit test that can assert that there are multiple segments. Not that it is a better test than the integration test, but it could catch some odd cases years from now when your integration test doesn't notice that it is running against a single segment due to, well, who knows what. I'm coming at this from pure paranoia. Mostly because I might make some crazy change years from now that invalidates the test and we never know it.
| .setSource("@timestamp", now + (long) i * numberOfDocsPerRefresh + j, "val", (double) j, "dim", String.valueOf(i)) | ||
| ); | ||
| } | ||
| assertNoFailures(bulkRequestBuilder.get()); |
There was a problem hiding this comment.
This won't really "make sure" we have a few segments - just make it quite likely. I'm guessing if you were to run an index stats after this to confirm we had a few segments it'd fail some percent of the time. Its fine, I think, but maybe comment is optimistic.
| } | ||
| LogManager.getLogger(SearchCancellationIT.class).info("Blocking in reduce"); | ||
| if (shouldBlock.get()) { | ||
| LogManager.getLogger(SearchCancellationIT.class).info("Blocking in reduce"); |
There was a problem hiding this comment.
Let's just declare a private static final Logger logger = LogManager.getLogger()?
|
maybe remove |
|
@elasticmachine run elasticsearch-ci/part-2 |
|
@elasticmachine update branch |
…ijun/elasticsearch into fix-none-tsdb-index-dimension-tests * 'fix-none-tsdb-index-dimension-tests' of github.com:weizijun/elasticsearch: (37 commits) [docs] Mention JDK 17 in the Contributing docs (elastic#84018) Fix GeoIpDownloader startup during rolling upgrade (elastic#84000) Script: Fields API for Dense Vector (elastic#83550) Move InferenceConfigUpdate under VersionedNamedWriteable (elastic#84022) [ML] Fix license feature test cleanup (elastic#84020) Replace deprecated api in artifact transforms (elastic#84015) QL: Add leniency option to SQL CLI (elastic#83795) [Stack Monitoring] add kibana_stats version alias to -mb template (elastic#83930) Optimize spliterator for ImmutableOpenMap (elastic#83899) Feature usage actions for archive (elastic#83931) Use latch to speedup multi feature migration test (elastic#84007) Make action names available in NodeClient (elastic#83919) [DOCS] Re-add HTTP proxy setings from elastic#82737 (elastic#84001) Add CI matrix configuration for snapshot BWC versions (elastic#83990) Update YAML Rest tests to check for product header on all responses (elastic#83290) TSDB: Add time series aggs cancellation (elastic#83492) [DOCS] Fix percolate query headings (elastic#83988) [DOCS] Move tip for percolate query example (elastic#83972) Simplify LocalExporter cleaner function to fix failing tests (elastic#83812) [GCE Discovery] Correcly handle large zones with 500 or more instances (elastic#83785) ...
4 participants
Adds support for low-level cancelling time-series based aggregations before
they reach the reduce phase.
Relates to #74660