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

Add hook for obtaining the final SearchResponse before sending to client #19587

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@polyfractal
Member

polyfractal commented Jul 25, 2016

Adds a hook which allows plugins to inspect the final SearchResponse (and the SearchRequest that generated it) before it is sent back to the client. This is distinct from per-shard hooks such as registering a SearchOperationListener, since it runs only on the coordinating node and gives access to the final, merged results.

It's a little more invasive than I had hoped, since I needed to pass the PluginService through the various Search Actions. But the actual change is pretty minor (I think).

@nik9000

View changes

Show outdated Hide outdated ...a/org/elasticsearch/action/search/SearchDfsQueryAndFetchAsyncAction.java

@polyfractal polyfractal removed the review label Jul 25, 2016

@polyfractal

This comment has been minimized.

Show comment
Hide comment
@polyfractal

polyfractal Aug 3, 2016

Member

@nik9000 Ok, how about now? Heading in the right direction now?

If this is more on the right track, I'll do some cleanup, comments, add tests, etc. Related, the naming is awfully clunky (but descriptive at least), and I wasn't sure the best place to put the interface.

Member

polyfractal commented Aug 3, 2016

@nik9000 Ok, how about now? Heading in the right direction now?

If this is more on the right track, I'll do some cleanup, comments, add tests, etc. Related, the naming is awfully clunky (but descriptive at least), and I wasn't sure the best place to put the interface.

@nik9000

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/plugins/PluginsService.java
@nik9000

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/plugins/SearchPlugin.java
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 4, 2016

Contributor

I think this is the right direction, yeah!

Contributor

nik9000 commented Aug 4, 2016

I think this is the right direction, yeah!

@s1monw

View changes

Show outdated Hide outdated ...java/org/elasticsearch/action/search/SearchResponseListenerRegistry.java
@s1monw

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/plugins/Plugin.java
@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Aug 4, 2016

Contributor

@polyfractal I left some suggestions to simplify this

Contributor

s1monw commented Aug 4, 2016

@polyfractal I left some suggestions to simplify this

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Aug 4, 2016

Contributor

@polyfractal maybe we do this very very simple. We a new extension point to SearchPlugin#getSearchResponseListener() returning BiFunction<SearchRequest, SearchResponse>. Then we pass this on to TransportSearchAction and wrap the action listener if there is at least one response listener and that way we have it integrated in all places and keep the change minimal? WDYT?

Contributor

s1monw commented Aug 4, 2016

@polyfractal maybe we do this very very simple. We a new extension point to SearchPlugin#getSearchResponseListener() returning BiFunction<SearchRequest, SearchResponse>. Then we pass this on to TransportSearchAction and wrap the action listener if there is at least one response listener and that way we have it integrated in all places and keep the change minimal? WDYT?

@polyfractal

This comment has been minimized.

Show comment
Hide comment
@polyfractal

polyfractal Aug 18, 2016

Member

Third time's a charm! Sorry for the delay, got caught up in some other work. Refactored to:

  • Remove the Guice injection of SearchPhaseController and instead instantiate manually in SearchModule
  • Use a List of BiConsumers for the listeners
  • Wrap the listener in TransportSearchAction instead of fiddling with each action individually

The de-guicing of SearchPhaseController meant the constructor of SearchModule had to change, which touched a lot of tests. I did my best to mock out the new constructor args appropriately -- and the tests seem to pass -- but I admit to largely just stumbling my way through it. :/

Member

polyfractal commented Aug 18, 2016

Third time's a charm! Sorry for the delay, got caught up in some other work. Refactored to:

  • Remove the Guice injection of SearchPhaseController and instead instantiate manually in SearchModule
  • Use a List of BiConsumers for the listeners
  • Wrap the listener in TransportSearchAction instead of fiddling with each action individually

The de-guicing of SearchPhaseController meant the constructor of SearchModule had to change, which touched a lot of tests. I did my best to mock out the new constructor args appropriately -- and the tests seem to pass -- but I admit to largely just stumbling my way through it. :/

new MockBigArrays(Settings.EMPTY, new NoneCircuitBreakerService()),
newTestScriptModule().getScriptService(), mock(ClusterService.class));
List<BiConsumer<SearchRequest, SearchResponse>> listeners = module.getSearchResponseListners();

This comment has been minimized.

@s1monw

s1monw Aug 18, 2016

Contributor

maybe assert that is't actually the one you added?

@s1monw

s1monw Aug 18, 2016

Contributor

maybe assert that is't actually the one you added?

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Aug 18, 2016

Contributor

i think this looks great. the only think I am missing is a test that actually calls one of those listeners. I think you have to add an Integration test that actually installs a plugin etc.

Contributor

s1monw commented Aug 18, 2016

i think this looks great. the only think I am missing is a test that actually calls one of those listeners. I think you have to add an Integration test that actually installs a plugin etc.

@polyfractal

This comment has been minimized.

Show comment
Hide comment
@polyfractal

polyfractal Sep 13, 2016

Member

Let this sit too long and much of it conflicts with the changes in #20423

I'll revisit the hook at a future date when I have more time, just juggling too many balls in the air right now. Sorry for the noise and needless reviews :/

Member

polyfractal commented Sep 13, 2016

Let this sit too long and much of it conflicts with the changes in #20423

I'll revisit the hook at a future date when I have more time, just juggling too many balls in the air right now. Sorry for the noise and needless reviews :/

jimczi added a commit that referenced this pull request Jan 19, 2017

Add the ability to define search response listeners in plugins (#22682)
This change is a simple adaptation of #19587 for the current state of master.
It allows to define search response listener in the form of `BiConsumer<SearchRequest, SearchResponse>`s in a search plugin.

jimczi added a commit that referenced this pull request Jan 19, 2017

Add the ability to define search response listeners in plugins (#22682)
This change is a simple adaptation of #19587 for the current state of master.
It allows to define search response listener in the form of `BiConsumer<SearchRequest, SearchResponse>`s in a search plugin.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment