[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky #8963

reta · 2023-07-28T13:43:49Z

Description

CompletionSuggestSearchIT.testSkipDuplicates is Flaky, caused by #8668

Related Issues

Resolves #8932

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Andriy Redko <[email protected]>

nknize

Thx @reta! I just saw this failure on #8826 and noticed the suggestion scores were wonky in the repro (not a flaky test!).

reta · 2023-07-28T14:24:18Z

Thx @reta! I just saw this failure on #8826 and noticed the suggestion scores were wonky in the repro (not a flaky test!).

Thanks @nknize, this is related to Apache Lucene 9.8.0-snapshot which is for main only (not backported to any of 2.x)

github-actions · 2023-07-28T14:26:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/21263/
CommitID: ab6c585

nknize

d0h.. something was bothering me a bit about this and I forgot about a Lucene deprecation made in 9.7. I updated my review. The fix is a smaller one liner but we'll want to update the rest of the codebase w/ that deprecation change as well.

nknize · 2023-07-28T14:26:39Z

server/src/main/java/org/opensearch/search/suggest/completion/CompletionSuggester.java

        for (LeafReaderContext context : searcher.getIndexReader().leaves()) {
            BulkScorer scorer = weight.bulkScorer(context);
            if (scorer != null) {
+                LeafCollector leafCollector = null;


I just realized this fix is simpler.

Per this Query#rewrite deprecation just change line 106 to the following

query = (CompletionQuery) query.rewrite(searcher);

You're basically reimplementing here the finish hook that was added in Lucene PR#12380.

That logic path wasn't being executed though because we're using the old collector API through the creation of a new searcher wrapping the reader.

I forgot about this deprecation in the 9.7 upgrade and it wasn't exposed because #12380 wasn't merged until after 9.7.

Sorry about it, merged just before, there is another issue I run into, again with Suggester but slightly different cause:

java.lang.IllegalArgumentException: onlrtdbizi is not a SuggestField at org.apache.lucene.search.suggest.document.CompletionWeight.bulkScorer(CompletionWeight.java:80) at org.opensearch.search.suggest.completion.CompletionSuggester.suggest(CompletionSuggester.java:110) at org.opensearch.search.suggest.completion.CompletionSuggester.innerExecute(CompletionSuggester.java:78) at org.opensearch.search.suggest.completion.CompletionSuggester.innerExecute(CompletionSuggester.java:1)

I could incorporate your suggestion into the upcoming fix (still trying to figure out what is happening)

I'm finishing a PR that cuts over all rewrites to the non-deprecated one. It includes this change in the CompletionSuggester and other places.

I opened the PR but I left your patch in. I was wrong, there's still an issue and cutting over to invoke the hook doesn't solve the occasional failure. Let me know if that's what you're investigating.

I opened the PR but I left your patch in. I was wrong, there's still an issue and cutting over to invoke the hook doesn't solve the occasional failure.

Do you have a sample stack trace or logs by any chance?

Let me know if that's what you're investigating.

The one that caused another batch of flakyness: #8968

nknize · 2023-07-28T14:30:16Z

d0h.. 2 minutes too late :)

I'll open a follow up PR to refactor all of the calls to the deprecated method.

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]>

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Kaushal Kumar <[email protected]>

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Ivan Brusic <[email protected]>

andrross · 2023-11-14T18:59:56Z

@reta We're seeing test failures on the 2.x branch (#8932 has been reopened), and I believe this change was never backported. Do we need to backport this?

reta · 2023-11-14T19:20:24Z

@andrross totally, I missed that once we moved 2.x to Apache Lucene 9.8.0

Signed-off-by: Andriy Redko <[email protected]> (cherry picked from commit 106e83a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…11200) (cherry picked from commit 106e83a) Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky

ab6c585

Signed-off-by: Andriy Redko <[email protected]>

reta added the skip-changelog label Jul 28, 2023

reta requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, dblock, dbwiddis, dreamer-89, gbbafna, kartg, kotwanikunal, mch2, nknize, owaiskazi19, ryanbogan, sachinpkale, saratvemulapalli, setiah, shwetathareja, sohami and tlfeng as code owners July 28, 2023 13:43

nknize approved these changes Jul 28, 2023

View reviewed changes

nknize added bug Something isn't working v2.10.0 labels Jul 28, 2023

reta removed the v2.10.0 label Jul 28, 2023

reta merged commit 106e83a into opensearch-project:main Jul 28, 2023

nknize suggested changes Jul 28, 2023

View reviewed changes

nknize mentioned this pull request Jul 28, 2023

Replace deprecated Query#rewrite(IndexReader with rewrite(IndexSearcher #8967

Merged

reta mentioned this pull request Jul 28, 2023

Revert "amending tests to include all codecs (#8907)" #8968

Merged

6 tasks

baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 28, 2023

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky (opensear…

b91e7e7

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]>

baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky (opensear…

f7ece2d

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]>

andrross mentioned this pull request Aug 3, 2023

[AUTOCUT] Gradle Check Failure on push to main #8935

Closed

brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky (opensear…

b3c5267

…ch-project#8963) Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Ivan Brusic <[email protected]>

reta added the backport 2.x Backport to 2.x branch label Nov 14, 2023

opensearch-trigger-bot bot mentioned this pull request Nov 14, 2023

[Backport 2.x] [BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky #11200

Merged

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky #8963

[BUG] CompletionSuggestSearchIT.testSkipDuplicates is flaky #8963

Uh oh!

Conversation

reta commented Jul 28, 2023

Description

Related Issues

Check List

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

reta commented Jul 28, 2023

Uh oh!

github-actions bot commented Jul 28, 2023

Gradle Check (Jenkins) Run Completed with:

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

nknize Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reta Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

nknize Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

nknize Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

reta Jul 28, 2023

Choose a reason for hiding this comment

Uh oh!

nknize commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrross commented Nov 14, 2023

Uh oh!

reta commented Nov 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nknize Jul 28, 2023 •

edited

Loading

nknize commented Jul 28, 2023 •

edited

Loading