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.

Sign up for GitHub

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

Jump to bottom

Reproducible failure in RareTermsAggregatorTests #64356

Closed
romseygeek opened this issue Oct 29, 2020 · 8 comments · Fixed by #64366
Closed

Reproducible failure in RareTermsAggregatorTests #64356

romseygeek opened this issue Oct 29, 2020 · 8 comments · Fixed by #64366
Assignees
@nik9000
Labels
:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test-failure Triaged test failures from CI v8.0.0-alpha1

Comments

@romseygeek
Copy link
Contributor

Found by CI on an unrelated pull request

Build scan:
https://gradle-enterprise.elastic.co/s/wj26schfjwofg/console-log?task=:server:test

Repro line:

./gradlew ':server:test' --tests "org.elasticsearch.search.aggregations.bucket.terms.RareTermsAggregatorTests.testNestedTerms" -Dtests.seed=EDABA787C024041A -Dtests.security.manager=true -Dtests.locale=id -Dtests.timezone=Europe/Zurich -Druntime.java=11

Reproduces locally?: Yes

Applicable branches: master

Failure history:
Nothing found in gradle enterprise

Failure excerpt:

org.elasticsearch.search.aggregations.bucket.terms.RareTermsAggregatorTests > testNestedTerms FAILED |  
-- | --
  | java.lang.AssertionError: segment should have at least one document to replay, got 0 |  
  | at __randomizedtesting.SeedInfo.seed([EDABA787C024041A:37E876EF4248F7B6]:0) |  
  | at org.elasticsearch.search.aggregations.bucket.BestBucketsDeferringCollector.prepareSelectedBuckets(BestBucketsDeferringCollector.java:162) |  
  | at org.elasticsearch.search.aggregations.bucket.DeferableBucketAggregator.prepareSubAggs(DeferableBucketAggregator.java:96) |  
  | at org.elasticsearch.search.aggregations.bucket.BucketsAggregator.buildSubAggsForBuckets(BucketsAggregator.java:189) |  
  | at org.elasticsearch.search.aggregations.bucket.BucketsAggregator.buildSubAggsForAllBuckets(BucketsAggregator.java:255) |  
  | at org.elasticsearch.search.aggregations.bucket.terms.StringRareTermsAggregator.buildAggregations(StringRareTermsAggregator.java:171) |  
  | at org.elasticsearch.search.aggregations.Aggregator.buildTopLevel(Aggregator.java:154) |  
  | at org.elasticsearch.search.aggregations.AggregatorTestCase.searchAndReduce(AggregatorTestCase.java:430) |  
  | at org.elasticsearch.search.aggregations.AggregatorTestCase.searchAndReduce(AggregatorTestCase.java:367) |  
  | at org.elasticsearch.search.aggregations.bucket.terms.RareTermsAggregatorTests.executeTestCase(RareTermsAggregatorTests.java:581) |  
  | at org.elasticsearch.search.aggregations.bucket.terms.RareTermsAggregatorTests.testSearchCase(RareTermsAggregatorTests.java:552) |  
  | at org.elasticsearch.search.aggregations.bucket.terms.RareTermsAggregatorTests.testNestedTerms(RareTermsAggregatorTests.java:333)
@romseygeek romseygeek added :Analytics/Aggregations Aggregations >test-failure Triaged test failures from CI v8.0.0 labels Oct 29, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 29, 2020
@nik9000 nik9000 self-assigned this Oct 29, 2020
@nik9000
Copy link
Member

nik9000 commented Oct 29, 2020

Sounds like something I caused. I'll have a look soon.

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2020

I'm not sure I caused this, but I believe I've tracked it down. I'll make a PR.

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2020

Oh yeah, I broke this yesterday. Explanation in PR.

@jtibshirani
Copy link
Contributor

Another instance just popped up in CI: https://gradle-enterprise.elastic.co/s/dejnmsrxyylnq. Depending on how long the PR takes to review + merge, we could mute the test immediately ?

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2020

we could mute the test immediately ?

I'm working through checkstyle now. So I'll have it open pretty soon. I'm not sure how long to review. It isn't super big.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Oct 29, 2020
@nik9000
Fixup rare terms
8aa6612 
In elastic#64016 I caused a bug in rare_terms where we would rewrite the
current leaf and remove all hits from it, causing us to trip an
assertion. This didn't happen before because previously we never rewrote
the current leaf. The fix involves cleaning up the state that the
deferring collector uses if we end up removing all hits.

This requires some super intimate knowledge of how
`BestBucketsDeferringCollector` works so I decided to move the
implementation of that mergin from `MergingBucketsDeferringCollector`
into `BestBucketsDeferringCollector`. This means that
`MergingBucketsDeferringCollector` is pretty much empty now. So I've
deprecated it and will remove it entirely in an subsequent change.

Closes elastic#64356
@nik9000 nik9000 mentioned this issue Oct 29, 2020
Merged
nik9000 added a commit that referenced this issue Oct 29, 2020
@nik9000
Fixup rare terms (#64366)
b4b01ec 
In #64016 I caused a bug in rare_terms where we would rewrite the
current leaf and remove all hits from it, causing us to trip an
assertion. This didn't happen before because previously we never rewrote
the current leaf. The fix involves cleaning up the state that the
deferring collector uses if we end up removing all hits.

This requires some super intimate knowledge of how
`BestBucketsDeferringCollector` works so I decided to move the
implementation of that mergin from `MergingBucketsDeferringCollector`
into `BestBucketsDeferringCollector`. This means that
`MergingBucketsDeferringCollector` is pretty much empty now. So I've
deprecated it and will remove it entirely in an subsequent change.

Closes #64356
@rjernst rjernst mentioned this issue Oct 29, 2020
Closed
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Oct 30, 2020
@nik9000
Fixup rare terms (backport of elastic#64366)
c76b0f1 
In elastic#64016 I caused a bug in rare_terms where we would rewrite the
current leaf and remove all hits from it, causing us to trip an
assertion. This didn't happen before because previously we never rewrote
the current leaf. The fix involves cleaning up the state that the
deferring collector uses if we end up removing all hits.

This requires some super intimate knowledge of how
`BestBucketsDeferringCollector` works so I decided to move the
implementation of that mergin from `MergingBucketsDeferringCollector`
into `BestBucketsDeferringCollector`. This means that
`MergingBucketsDeferringCollector` is pretty much empty now. So I've
deprecated it and will remove it entirely in an subsequent change.

Closes elastic#64356
@nik9000 nik9000 mentioned this issue Oct 30, 2020
Merged
@nik9000
Copy link
Member

nik9000 commented Oct 30, 2020

Sorry for fixing this in master, closing the issue, and then not opening the backport over night.

nik9000 added a commit that referenced this issue Oct 30, 2020
@nik9000
Fixup rare terms (backport of #64366) (#64416)
1181b96 
In #64016 I caused a bug in rare_terms where we would rewrite the
current leaf and remove all hits from it, causing us to trip an
assertion. This didn't happen before because previously we never rewrote
the current leaf. The fix involves cleaning up the state that the
deferring collector uses if we end up removing all hits.

This requires some super intimate knowledge of how
`BestBucketsDeferringCollector` works so I decided to move the
implementation of that mergin from `MergingBucketsDeferringCollector`
into `BestBucketsDeferringCollector`. This means that
`MergingBucketsDeferringCollector` is pretty much empty now. So I've
deprecated it and will remove it entirely in an subsequent change.

Closes #64356
@nik9000
Copy link
Member

nik9000 commented Oct 30, 2020

All backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nik9000 nik9000

Labels
:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test-failure Triaged test failures from CI v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixup rare terms nik9000/elasticsearch
5 participants
@nik9000 @jakelandis @romseygeek @jtibshirani @elasticmachine