Make fetch sub phases pluggable #12400

brwe · 2015-07-22T16:07:08Z

This is wip, only opening this pull request so that it is easier to discuss.
I made the fetch sub phases pluggable to see if this adds too much complexity. With this change new or existing fetch sub phases such as fielddata fields or script fields can be plugged in.
As a proof of concept I changed fielddata fields to use the plugin mechanism and added an example for
a plugged in fetch phase in the test.
While this adds a little more guice stuff, it also removes the need for implementing methods like hasFieldDataFields() and fieldDataFields() in every subclass of SearchContext.
In addition we could easily add features like #10729 and also move features like fielddatafields from core to a plugin immediately. Overall my feeling is that it would be a win to make sub phases pluggable.

It is work in progress because converting inner hits to use the plugin mechanism will be a little more
tricky. Also not all fetch phases are structured the same so converting all sub phases might need more work as well.

brwe · 2015-07-22T16:20:43Z

@clintongormley could you take a look and let me know what you think?

jpountz · 2015-07-22T17:06:09Z

I haven't looked at the pull request yet, but for more context: I was first on the fence about adding more entry points to plugins, until Britta brought up the selling point that it would allow to move some current fetch sub phases, like fielddata fields, to plugins, which I like as it would help keep the core small and focused on major features (see #10368).

martijnvg · 2015-07-22T17:18:54Z

+1 to make sub fetch phases pluggable. If sub fetch phase implementations are better isolated, then this will automatically improve the design and code of these sub fetch phase implementations.

jpountz · 2015-07-23T13:10:03Z

core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

+        for (FetchSubPhase phase : fetchSubPhases) {
+            this.fetchSubPhases[counter] = phase;
+            counter++;
+        }


or just fetchSubPhases.toArray(this.fetchSubPhases)?

jpountz · 2015-07-23T13:27:05Z

It looks good to me overall. Maybe @martijnvg and @dakrone should have a look since they worked on inner hits (which is handled differently in your PR) and fielddata fields.

One thing I'm wondering when looking at the changes is whether we actually need hasFetchSubPhaseContext. It looks to me that we always do something like: if (hasSubFetchPhase) { execute fetch sub phase } so maybe we could just have fetch sub phase execution be smart enough to not do anything if it has not been configured? This would help limit the number of methods we need on the SearchContext class.

dakrone · 2015-07-24T17:55:23Z

core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseContext.java

+
+package org.elasticsearch.search.fetch;
+
+public interface FetchSubPhaseContext {


Can you document this class?

dakrone · 2015-07-24T18:11:51Z

left some comments, I agree with Adrien's comment about potentially getting rid of hasFetchSubPhaseContext, I think it would be a cleaner interface. Other than that I like this!

brwe · 2015-07-27T10:08:33Z

Thanks a lot for the feedback! I removed hasFetchSubPhaseContext() now. Because I still need some way to let the fetch sub phase know if it should execute or not I now put this information in the FetchSubPhaseContext and the FetchSubPhaseParseElement makes sure that this is always set when the sub phase is parsed. Let me know if this makes sense.
I also added documentation - did not mean to make anyone sad :)

To proceed: Do we want to merge this after review cycles are done and then later convert each of the other sub phases in separate pull requests? Or should I do all at once?

nik9000 · 2015-07-28T19:44:20Z

To proceed: Do we want to merge this after review cycles are done and then later convert each of the other sub phases in separate pull requests? Or should I do all at once?

I'm all for doing things incrementally. Less long lived branches the better.

nik9000 · 2015-07-28T19:51:14Z

core/src/main/java/org/elasticsearch/search/fetch/fielddata/FieldDataFieldsParseElement.java

    @Override
-    public void parse(XContentParser parser, SearchContext context) throws Exception {
+    protected void innerParse(XContentParser parser, FetchSubPhaseContext fetchSubPhaseContext) throws Exception {
+        FieldDataFieldsContext fieldDataFieldsContext = (FieldDataFieldsContext) fetchSubPhaseContext;


Maybe FetchSubPhaseParseElement should be parameterized?

I'm +1 if it fits nicely, but otherwise I'd like us to be cautious with generics. Our code base probably makes too much use of generics today, to a point where they don't help avoid casting anymore.

I'll make it parameterized and then we can decide.

martijnvg · 2015-07-29T08:39:16Z

+1 this looks good. I'm good with inner hits being handled differently than all the other sub fetch phases.

brwe · 2015-07-29T15:47:54Z

Thanks again for the feedback! I did my best to parameterize all places where I had casts before. It works but I am unsure if this is the right way. Can you take another critical look?

nik9000 · 2015-07-29T16:46:02Z

core/src/test/java/org/elasticsearch/search/fetch/FetchSubPhasePluginTests.java

+        public Map<String, ? extends SearchParseElement> parseElements() {
+            ImmutableMap.Builder<String, SearchParseElement> parseElements = ImmutableMap.builder();
+            parseElements.put("term_vectors_fetch", new TermVectorsFetchParseElement());
+            return parseElements.build();


I'd write this ImmutableMap.of("term_vectors_fetch", new TermVectorsFetchParseElement());. Not that it matters either way.

brwe · 2015-07-30T15:58:08Z

@nik9000 thanks again for the comments! updated pr and happily awaiting next review round

nik9000 · 2015-07-30T18:18:58Z

core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

-        this.fetchSubPhases = new FetchSubPhase[]{scriptFieldsPhase, matchedQueriesPhase, explainPhase, highlightPhase,
-                fetchSourceSubPhase, versionPhase, fieldDataFieldsFetchSubPhase, innerHitsFetchSubPhase};
+        this.fetchSubPhases = fetchSubPhases.toArray(new FetchSubPhase[fetchSubPhases.size() + 1]);
+        this.fetchSubPhases[fetchSubPhases.size()] = innerHitsFetchSubPhase;


Did you want to add sorting here? I forget what the outcome of that discussion was. Right now the order is random and that might be wrong for some use cases. Or maybe we just wait until we see such use cases.

I could not think of any use case and I believe the order was random before. Tests pass so I guess that is OK?

Find by me.

nik9000 · 2015-07-30T18:21:22Z

Ok - I left one comment but it can wait. I feel good about this.

jpountz · 2015-08-03T13:17:13Z

LGTM

nik9000 · 2015-08-03T13:18:01Z

LGTM

Make fetch sub phases pluggable

brwe · 2015-08-03T13:21:53Z

Uh. forgot to squash before merging. sorry...will remember next time.

brwe added WIP discuss labels Jul 22, 2015

brwe mentioned this pull request Jul 22, 2015

Return term vectors as part of the search response #10729

Closed

clintongormley added >enhancement :Search/Search Search-related issues that do not fall into other categories labels Jul 23, 2015

jpountz reviewed Jul 23, 2015
View reviewed changes

dakrone reviewed Jul 24, 2015
View reviewed changes

brwe removed the WIP label Jul 27, 2015

brwe added the v2.0.0-beta1 label Jul 27, 2015

brwe mentioned this pull request Jul 27, 2015

Remove unused methods hitsExecutionNeeded and hitsExecute from FetchSubPhase #12476

Closed

nik9000 reviewed Jul 28, 2015
View reviewed changes

brwe added 4 commits July 29, 2015 15:44

make fetch sub phases pluggable

bcebb61

make fielddata fields a plugin

5f0f669

toArray

df339f4

remove hasFetchSubPhaseContext

ea14f67

brwe added 4 commits July 29, 2015 15:44

remove unused imports and vars

3ee3238

add some documentation

24c4b27

make Context generic so we don't have to cast

6295738

add SearchContext to innerParse method

d113b59

brwe force-pushed the plug-fetch-sub-phases branch from 839f473 to d113b59 Compare July 29, 2015 14:13

more generic?

6754a26

brwe force-pushed the plug-fetch-sub-phases branch from 7ec0174 to 1397ff4 Compare July 29, 2015 15:35

generic for sub classes also?

b86d07f

brwe force-pushed the plug-fetch-sub-phases branch from 1397ff4 to b86d07f Compare July 29, 2015 15:37

nik9000 reviewed Jul 29, 2015
View reviewed changes

more review comments

8a49579

nik9000 reviewed Jul 30, 2015
View reviewed changes

brwe added a commit that referenced this pull request Aug 3, 2015

Merge pull request #12400 from brwe/plug-fetch-sub-phases

26d51c2

Make fetch sub phases pluggable

brwe merged commit 26d51c2 into elastic:master Aug 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fetch sub phases pluggable #12400

Make fetch sub phases pluggable #12400

brwe commented Jul 22, 2015

brwe commented Jul 22, 2015

jpountz commented Jul 22, 2015

martijnvg commented Jul 22, 2015

jpountz Jul 23, 2015

jpountz commented Jul 23, 2015

dakrone Jul 24, 2015

dakrone commented Jul 24, 2015

brwe commented Jul 27, 2015

nik9000 commented Jul 28, 2015

nik9000 Jul 28, 2015

jpountz Jul 29, 2015

brwe Jul 29, 2015

martijnvg commented Jul 29, 2015

brwe commented Jul 29, 2015

nik9000 Jul 29, 2015

brwe commented Jul 30, 2015

nik9000 Jul 30, 2015

brwe Aug 3, 2015

nik9000 Aug 3, 2015

nik9000 commented Jul 30, 2015

jpountz commented Aug 3, 2015

nik9000 commented Aug 3, 2015

brwe commented Aug 3, 2015


		package org.elasticsearch.search.fetch;

		public interface FetchSubPhaseContext {

Make fetch sub phases pluggable #12400

Make fetch sub phases pluggable #12400

Conversation

brwe commented Jul 22, 2015

brwe commented Jul 22, 2015

jpountz commented Jul 22, 2015

martijnvg commented Jul 22, 2015

Choose a reason for hiding this comment

jpountz commented Jul 23, 2015

Choose a reason for hiding this comment

dakrone commented Jul 24, 2015

brwe commented Jul 27, 2015

nik9000 commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jul 29, 2015

brwe commented Jul 29, 2015

Choose a reason for hiding this comment

brwe commented Jul 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Jul 30, 2015

jpountz commented Aug 3, 2015

nik9000 commented Aug 3, 2015

brwe commented Aug 3, 2015