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 IndexSearcherWrapper visibility #39071

Merged
merged 4 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@jimczi
Copy link
Member

commented Feb 18, 2019

This change adds a wrapper for IndexSearcher that makes IndexSearcher#search(List, Weight, Collector) visible by sub-classes. The wrapper is used by the ContextIndexSearcher to call this protected method on a searcher created by a plugin.
This ensures that an override of the protected method in an IndexSearcherWrapper plugin is called when a search is executed.

Closes #30758

Fix IndexSearcherWrapper visibility
This change adds a wrapper for IndexSearcher that makes IndexSearcher#search(List, Weight, Collector) visible by
sub-classes. The wrapper is used by the ContextIndexSearcher to call this protected method on a searcher created by a plugin.
This ensures that an override of the protected method in an IndexSearcherWrapper plugin is called when a search is executed.

Closes #30758
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2019

jimczi added some commits Feb 18, 2019

@mayya-sharipova
Copy link
Contributor

left a comment

@jimczi Thanks Jim, makes sense.

protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector) throws IOException {
// we cannot use the asserting searcher because the weight is created by the ContextIndexSearcher
wrappedSearcher.search(leaves, weight, collector);
}

This comment has been minimized.

Copy link
@mayya-sharipova

mayya-sharipova Feb 26, 2019

Contributor

I understand we don't want to call AssertingIndexSearcher::search as it will fail, as it expects AssertingWeight.
But I am wondering if we are losing anything by not calling search method of AssertingIndexSearcher? Or it doesn't matter, cause even before this PR when AssertingIndexSearcher is wrapped in ContextIndexSearcher we were not calling AssertingIndexSearcher::search anyway?

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 15, 2019

Contributor

Tricky tricky, I can't think of a way to work around this?

This comment has been minimized.

Copy link
@jimczi

jimczi Mar 15, 2019

Author Member

Yep me neither, it's not a regression though since the current code is also not using AssertingIndexSearcher::search. I think we need a better integration of the ContextIndexSearcher if you're willing to keep the ability to extend IndexSearcher in a plugin but this is probably out of the scope of this pr that I consider as a workaround (hack ;)).

@jimczi jimczi requested a review from jpountz Mar 15, 2019

@jpountz
Copy link
Contributor

left a comment

Mayya has a good point, but I can't think of a way to address it. Other than that the change looks good to me.

protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector) throws IOException {
// we cannot use the asserting searcher because the weight is created by the ContextIndexSearcher
wrappedSearcher.search(leaves, weight, collector);
}

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 15, 2019

Contributor

Tricky tricky, I can't think of a way to work around this?

@jimczi jimczi merged commit e4d46ba into elastic:master Mar 15, 2019

8 checks passed

CLA Commit author is a member of Elasticsearch
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/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@jimczi jimczi deleted the jimczi:bug/index_searcher_wrapper branch Mar 15, 2019

jimczi added a commit to jimczi/elasticsearch that referenced this pull request Mar 15, 2019

Fix IndexSearcherWrapper visibility (elastic#39071)
* Fix IndexSearcherWrapper visibility

This change adds a wrapper for IndexSearcher that makes IndexSearcher#search(List, Weight, Collector) visible by
sub-classes. The wrapper is used by the ContextIndexSearcher to call this protected method on a searcher created by a plugin.
This ensures that an override of the protected method in an IndexSearcherWrapper plugin is called when a search is executed.

Closes elastic#30758

jimczi added a commit to jimczi/elasticsearch that referenced this pull request Mar 15, 2019

Fix IndexSearcher wrappers interaction with AssertingIndexSearcher
This commit ensures that we delegate the creation of the query weight to the AssertingIndexSearcher
when possible (if the query does not need to access distributed frequency statistics). This fixes test failures
that uses a ContextIndexSearcher wrapped with an AssertingIndexSearcher.

Relates elastic#39071

jimczi added a commit that referenced this pull request Mar 15, 2019

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

I had to revert the pr because some tests fail when an AssertingIndexSearcher is used on a profiled query. I'll investigate further and will open a new pr if I find a proper fix.

@jimczi jimczi added the stalled label Mar 15, 2019

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 16, 2019

Merge remote-tracking branch 'elastic/master' into remove-cluster-sta…
…te-size-hard-failure

* elastic/master:
  use shell with JAVA_HOME for starting archive (elastic#40118)
  Remove Migration Upgrade and Assistance APIs (elastic#40075)
  Revert "Fix IndexSearcherWrapper visibility (elastic#39071)"
  Remove transport name from tcp channel (elastic#40074)
  Document the limitation around field aliases and percolator. (elastic#40073)
  SQL: Refactor Literals serialization method (elastic#40058)

jimczi added a commit that referenced this pull request Mar 18, 2019

jimczi added a commit to jimczi/elasticsearch that referenced this pull request Mar 18, 2019

Fix IndexSearcherWrapper visibility (elastic#39071)
This change adds a wrapper for IndexSearcher that makes IndexSearcher#search(List, Weight, Collector) visible by
sub-classes. The wrapper is used by the ContextIndexSearcher to call this protected method on a searcher created by a plugin.
This ensures that an override of the protected method in an IndexSearcherWrapper plugin is called when a search is executed.

Closes elastic#30758

@jimczi jimczi removed the stalled label Mar 18, 2019

jimczi added a commit that referenced this pull request Mar 18, 2019

Fix IndexSearcherWrapper visibility (#39071) (#40145)
This change adds a wrapper for IndexSearcher that makes IndexSearcher#search(List, Weight, Collector) visible by
sub-classes. The wrapper is used by the ContextIndexSearcher to call this protected method on a searcher created by a plugin.
This ensures that an override of the protected method in an IndexSearcherWrapper plugin is called when a search is executed.

Closes #30758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.