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

MatchingDirectoryReader should not use a threaded searcher #100527

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

romseygeek
Copy link
Contributor

@romseygeek romseygeek commented Oct 9, 2023

MatchingDirectoryReader is a test reader wrapper that filters out documents
matching a particular query. For each leaf, we create an IndexSearcher,
execute the query against it and then use that as a filter for the leaf. This searcher
is created using LuceneTestCase.newSearcher() and as such may be multi-
threaded, which triggers extra index checks. For tests that are expecting certain
methods to be called against internal readers a given number of times, these extra
checks can add additional calls which then lead to a failure of test assumptions.

Because this IndexSearcher is only executed against a single leaf it will only ever use
a single thread, and so we can explicitly disable threading here.

Fixes #100487
Fixes #99916

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.12.0 labels Oct 9, 2023
@romseygeek romseygeek requested a review from martijnvg October 9, 2023 15:46
@romseygeek romseygeek self-assigned this Oct 9, 2023
@romseygeek romseygeek added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories and removed needs:triage Requires assignment of a team area label labels Oct 9, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Oct 9, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@romseygeek romseygeek merged commit b286fb3 into elastic:main Oct 10, 2023
@romseygeek romseygeek deleted the bug/shard-get-service-tests branch October 10, 2023 09:38
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Oct 10, 2023
…00527)

MatchingDirectoryReader is a test reader wrapper that filters out documents
matching a particular query. For each leaf, we create an IndexSearcher,
execute the query against it and then use that as a filter for the leaf. This searcher
is created using LuceneTestCase.newSearcher() and as such may be multi-
threaded, which triggers extra index checks. For tests that are expecting certain
methods to be called against internal readers a given number of times, these extra
checks can add additional calls which then lead to a failure of test assumptions.

Because this IndexSearcher is only executed against a single leaf it will only ever use
a single thread, and so we can explicitly disable threading here.

Fixes elastic#100487
Fixes elastic#99916
romseygeek added a commit that referenced this pull request Oct 10, 2023
…100597)

MatchingDirectoryReader is a test reader wrapper that filters out documents
matching a particular query. For each leaf, we create an IndexSearcher,
execute the query against it and then use that as a filter for the leaf. This searcher
is created using LuceneTestCase.newSearcher() and as such may be multi-
threaded, which triggers extra index checks. For tests that are expecting certain
methods to be called against internal readers a given number of times, these extra
checks can add additional calls which then lead to a failure of test assumptions.

Because this IndexSearcher is only executed against a single leaf it will only ever use
a single thread, and so we can explicitly disable threading here.

Fixes #100487
Fixes #99916
Fixes #100460
@javanna
Copy link
Member

javanna commented Oct 10, 2023

Thanks for fixing this!

romseygeek added a commit that referenced this pull request Oct 11, 2023
…00668)

Follow up to #100527

We are not testing anything to do with searching with this searcher, and so
there is no point in using LuceneTestCase.newSearcher() which will wrap
it with all sorts of extra checks that may access the underlying reader in
ways that are not anticipated by tests.

Fixes #100460
Fixes #99024
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Oct 11, 2023
…astic#100668)

Follow up to elastic#100527

We are not testing anything to do with searching with this searcher, and so
there is no point in using LuceneTestCase.newSearcher() which will wrap
it with all sorts of extra checks that may access the underlying reader in
ways that are not anticipated by tests.

Fixes elastic#100460
Fixes elastic#99024
elasticsearchmachine pushed a commit that referenced this pull request Oct 11, 2023
…00668) (#100685)

Follow up to #100527

We are not testing anything to do with searching with this searcher, and so
there is no point in using LuceneTestCase.newSearcher() which will wrap
it with all sorts of extra checks that may access the underlying reader in
ways that are not anticipated by tests.

Fixes #100460
Fixes #99024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
: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.12.0
Projects
None yet
4 participants