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

Adjust mute for particular condition for 106647 #106675

Merged

Conversation

benwtrent
Copy link
Member

@benwtrent benwtrent commented Mar 22, 2024

The test failure occurs when we have a threshold. Even when only gathering a single document for a query, because of this line:

    public LeafCollector getLeafCollector(LeafReaderContext context) throws IOException {
        if (hitsThresholdChecker.totalHitsThreshold == Integer.MAX_VALUE) {
            return super.getLeafCollector(context);
        }
        earlyTerminateIfNeeded(); //<-- Terminate on segment before checking if there are any matching docs at all.

We will check for earlyTermination even if the leaf has no matching docs. But, this is expected by other tests.

This does a partial unmute allowing for the test to run, but acknowledging we have a strange edge case here that conflicts the expected results for two of our tests:

testCollectedHitCount() when threshold==1 vs
testEarlyTerminatesWithoutCollection()

@benwtrent benwtrent added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories auto-backport-and-merge v8.13.1 v8.14.0 v8.12.3 labels Mar 22, 2024
@benwtrent benwtrent requested a review from javanna March 22, 2024 13:32
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Mar 22, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@benwtrent
Copy link
Member Author

@javanna I am just going to close this as you expressed interest in fixing this yourself in a different way.

@benwtrent benwtrent closed this Apr 4, 2024
@javanna
Copy link
Member

javanna commented Apr 4, 2024

I meant to review this and better understand it, but I had no spare cycles yet. Did not necessarily have a different approach in mind. I had the feeling that we may be hiding bugs by adjusting the code, that's it.

@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent changed the title Fix test failure 106647 Adjust mute for particular condition for 106647 May 1, 2024
@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-backport-and-merge labels May 1, 2024
@benwtrent
Copy link
Member Author

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit 64a6a08 into elastic:main May 1, 2024
14 checks passed
@benwtrent benwtrent deleted the feature/fix-test-failure-106647 branch May 1, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants