Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix early termination of aggregators that run with breadth-first mode #44963

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@jimczi
Copy link
Member

commented Jul 29, 2019

This commit fixes a bug when a deferred aggregator tries to early terminate the collection. In such case the CollectionTerminatedException is not caught and
the search fails on the shard. This change makes sure that we catch the exception in order to continue the deferred collection on the next leaf.

Fixes #44909

This commit fixes a bug when a deferred aggregator tries to early terminate the collection. In such case the CollectionTerminatedException is not caught and
the search fails on the shard. This change makes sure that we catch the exception in order to continue the deferred collection on the next leaf.

Fixes #44909
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

.setTrackTotalHits(false)
.setQuery(matchAllQuery())
.addAggregation(max("max").field("values"))
.addAggregation(count("count").field("values"))

This comment has been minimized.

Copy link
@polyfractal

polyfractal Jul 29, 2019

Member

An unrelated question: is the count agg required here to check for correctness? It looks like count doesn't use the early termination optimization, so wasn't sure if that is being used as a proxy to make sure that other aggs are unaffected, or something?

Asking because @csoulios is converting the max IT to unit tests, and we were discussing the other early termination test last week. I wasn't quite sure the purpose of count here, or if it was included because why not :)

This comment has been minimized.

Copy link
@jimczi

jimczi Jul 29, 2019

Author Member

Yes, it is used to check if a sibling is not affected by an aggregation that terminates early. So the fact that we don't collect any hits for the max aggregation should not impact the count one which needs to see all hits.

This comment has been minimized.

Copy link
@polyfractal

polyfractal Jul 29, 2019

Member

Gotcha, thanks. So if we embedded this inside a filter agg with match_all it should be functionally the same, right?

AggregatorTestCase currently assumes a single top-level agg for simplicity, so when converting these IT over to unit tests we weren't sure how to handle it. We could also just leave the early termination tests as IT too.

(Note: not for this PR, just discussion regarding the changes we're making to tests in a separate PR)

This comment has been minimized.

Copy link
@jimczi

jimczi Jul 29, 2019

Author Member

So if we embedded this inside a filter agg with match_all it should be functionally the same, right?

Yes

Copy link
Member

left a comment

LGTM. Shame we have to use exceptions for control flow, but last time I looked at all of this it definitely seemed like the path of least resistance :)

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@elasticmachine run elasticsearch-ci/2

@jimczi jimczi merged commit e82818a into elastic:master Jul 30, 2019
8 checks passed
8 checks passed
CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docs Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@jimczi jimczi deleted the jimczi:breadth_first_early_termination branch Jul 30, 2019
jimczi added a commit that referenced this pull request Jul 30, 2019
This commit fixes a bug when a deferred aggregator tries to early terminate the collection. In such case the CollectionTerminatedException is not caught and
the search fails on the shard. This change makes sure that we catch the exception in order to continue the deferred collection on the next leaf.

Fixes #44909
jimczi added a commit that referenced this pull request Jul 30, 2019
This commit fixes a bug when a deferred aggregator tries to early terminate the collection. In such case the CollectionTerminatedException is not caught and
the search fails on the shard. This change makes sure that we catch the exception in order to continue the deferred collection on the next leaf.

Fixes #44909
jimczi added a commit that referenced this pull request Jul 30, 2019
This commit fixes a bug when a deferred aggregator tries to early terminate the collection. In such case the CollectionTerminatedException is not caught and
the search fails on the shard. This change makes sure that we catch the exception in order to continue the deferred collection on the next leaf.

Fixes #44909
jkakavas added a commit that referenced this pull request Jul 31, 2019
This commit fixes a bug when a deferred aggregator tries to early terminate the collection. In such case the CollectionTerminatedException is not caught and
the search fails on the shard. This change makes sure that we catch the exception in order to continue the deferred collection on the next leaf.

Fixes #44909
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.