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

Make fetch sub phases pluggable #12400

Merged
merged 11 commits into from Aug 3, 2015

Conversation

@brwe
Copy link
Contributor

commented Jul 22, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

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

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2015

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

This comment has been minimized.

Copy link
Member

commented Jul 22, 2015

+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
jpountz reviewed Jul 23, 2015
View changes
core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java Outdated
for (FetchSubPhase phase : fetchSubPhases) {
this.fetchSubPhases[counter] = phase;
counter++;
}

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 23, 2015

Contributor

or just fetchSubPhases.toArray(this.fetchSubPhases)?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2015

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
dakrone reviewed Jul 24, 2015
View changes
core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseContext.java Outdated

package org.elasticsearch.search.fetch;

public interface FetchSubPhaseContext {

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

Can you document this class?

@dakrone
dakrone reviewed Jul 24, 2015
View changes
core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java Outdated
@@ -114,4 +114,11 @@ public String getSourcePath(String sourcePath) {
boolean hitsExecutionNeeded(SearchContext context);

void hitsExecute(SearchContext context, InternalSearchHit[] hits);

public interface ContextFactory {

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

Can you add a javadoc for what this class is for (for plugin authors)

@dakrone
dakrone reviewed Jul 24, 2015
View changes
core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseModule.java Outdated
import java.util.List;

/**
*

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

No documentation makes me sad! :)

@dakrone
dakrone reviewed Jul 24, 2015
View changes
core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java Outdated
@@ -728,6 +728,11 @@ public Counter timeEstimateCounter() {
}

@Override
public boolean hasFetchSubPhaseContext(FetchSubPhase.ContextFactory contextFactory) {
return subPhaseContexts.get(contextFactory.getName()) != null;

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

This can just be return subPhaseContexts.containsKey(contextFactory.getName())

@dakrone
dakrone reviewed Jul 24, 2015
View changes
core/src/main/java/org/elasticsearch/search/internal/SearchContext.java Outdated
@@ -360,6 +360,8 @@ public void clearReleasables(Lifetime lifetime) {

public abstract Counter timeEstimateCounter();

public abstract boolean hasFetchSubPhaseContext(FetchSubPhase.ContextFactory contextFactory);

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

Since the implementation of this seems to be a check in the hash map, does it make sense to have it as a separate method instead of annotating getFetchSubPhaseContext with @Nullable?

assertThat(((Map<String, Integer>) response.getHits().getAt(0).field("term_vectors_fetch").getValues().get(0)).get("sam"), equalTo(1));
}

public static class FetchTermVectorsPlugin extends AbstractPlugin {

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

Nice to have an example of how to implement fetch phases as a plugin!

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 28, 2015

Contributor

+1 I like plugin examples!

@dakrone
dakrone reviewed Jul 24, 2015
View changes
core/src/test/java/org/elasticsearch/test/TestSearchContext.java Outdated

@Override
public boolean hasFetchSubPhaseContext(FetchSubPhase.ContextFactory contextFactory) {
return subPhaseContexts.get(contextFactory.getName()) != null;

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 24, 2015

Member

can use .containsKey here too

@dakrone

This comment has been minimized.

Copy link
Member

commented Jul 24, 2015

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 brwe removed the WIP label Jul 27, 2015
@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2015

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
nik9000 reviewed Jul 28, 2015
View changes
core/src/main/java/org/elasticsearch/search/fetch/fielddata/FieldDataFieldsParseElement.java Outdated
@Override
public void parse(XContentParser parser, SearchContext context) throws Exception {
protected void innerParse(XContentParser parser, FetchSubPhaseContext fetchSubPhaseContext) throws Exception {
FieldDataFieldsContext fieldDataFieldsContext = (FieldDataFieldsContext) fetchSubPhaseContext;

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 28, 2015

Contributor

Maybe FetchSubPhaseParseElement should be parameterized?

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 29, 2015

Contributor

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.

This comment has been minimized.

Copy link
@brwe

brwe Jul 29, 2015

Author Contributor

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

@nik9000
nik9000 reviewed Jul 28, 2015
View changes
core/src/main/java/org/elasticsearch/search/fetch/FetchSubPhaseParseElement.java Outdated
/**
* Implement the actual parsing here.
*/
protected abstract void innerParse(XContentParser parser, FetchSubPhaseContext fetchSubPhaseContext) throws Exception;

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 28, 2015

Contributor

Should this also take a SearchContext for lookups and things?

This comment has been minimized.

Copy link
@brwe

brwe Jul 29, 2015

Author Contributor

Indeed! I did not need it for the examples I did but it will be needed for the other fetch phases.

@martijnvg

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

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

brwe added 4 commits Jul 22, 2015
@brwe brwe force-pushed the brwe:plug-fetch-sub-phases branch to d113b59 Jul 29, 2015
@brwe brwe force-pushed the brwe:plug-fetch-sub-phases branch Jul 29, 2015
@brwe brwe force-pushed the brwe:plug-fetch-sub-phases branch to b86d07f Jul 29, 2015
@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2015

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?

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

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 29, 2015

Contributor

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

package org.elasticsearch.search.fetch;

/**
* This class stores if or if not a FetchSubPhase is supposed to execute.

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 29, 2015

Contributor

Maybe "All configuration and context needed by the FetchSubPhase to execute on hits. The only required information is whether or not the sub phase needs to be run at all." ?

@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

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

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;

This comment has been minimized.

Copy link
@nik9000

nik9000 Jul 30, 2015

Contributor

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.

This comment has been minimized.

Copy link
@brwe

brwe Aug 3, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@nik9000

nik9000 Aug 3, 2015

Contributor

Find by me.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2015

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

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

LGTM

1 similar comment
@nik9000

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

LGTM

brwe added a commit that referenced this pull request Aug 3, 2015
@brwe brwe merged commit 26d51c2 into elastic:master Aug 3, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.