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

Introduce a formal ExtensionPoint class to stream line extensions #12826

Merged
merged 1 commit into from Aug 13, 2015

Conversation

Projects
None yet
5 participants
@s1monw
Contributor

s1monw commented Aug 12, 2015

This commit tries to add some infrastructure to streamline how extension
points should be strucutred. It's a simple approache with 4 implementations
for highlighter, suggester, allocation_decider and shards_allocator.
It simplifies adding new extension points and forces to register classes instead
of strings.

@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Aug 12, 2015

Member

One thought here is to split ExtensionPoint into two different variants, one for a Set and one for a Map. Right now it looks like we have a switch for this, with if/else in the impl. We just have those two types of extension points, either we keep a set of class impls, or we keep a map from name to impl. I think separating them would make the module code a little clearer, and less error prone when adding these (must choose the type of extension when defining, instead of accidentally calling the wrong register method).

Member

rjernst commented Aug 12, 2015

One thought here is to split ExtensionPoint into two different variants, one for a Set and one for a Map. Right now it looks like we have a switch for this, with if/else in the impl. We just have those two types of extension points, either we keep a set of class impls, or we keep a map from name to impl. I think separating them would make the module code a little clearer, and less error prone when adding these (must choose the type of extension when defining, instead of accidentally calling the wrong register method).

@rjernst

View changes

Show outdated Hide outdated .../java/org/elasticsearch/cluster/routing/allocation/AllocationModule.java Outdated
@rjernst

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java Outdated
@rjernst

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/search/highlight/Highlighters.java Outdated
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.*;
/**

This comment has been minimized.

@rjernst

rjernst Aug 12, 2015

Member

Nit: Remove empty useless comment while we are here?

@rjernst

rjernst Aug 12, 2015

Member

Nit: Remove empty useless comment while we are here?

This comment has been minimized.

@dakrone

dakrone Aug 12, 2015

Member

Better yet, add javadocs! ;)

@dakrone

dakrone Aug 12, 2015

Member

Better yet, add javadocs! ;)

This comment has been minimized.

@s1monw

s1monw Aug 12, 2015

Contributor

done

@s1monw

s1monw Aug 12, 2015

Contributor

done

map.put("phrase", new PhraseSuggester(scriptService));
map.put("term", new TermSuggester());
map.put("completion", new CompletionSuggester());
map.putAll(suggesters);

This comment has been minimized.

@rjernst

rjernst Aug 12, 2015

Member

Can we ensure the passed in suggester names don't collide with the builtins?

@rjernst

rjernst Aug 12, 2015

Member

Can we ensure the passed in suggester names don't collide with the builtins?

This comment has been minimized.

@s1monw

s1monw Aug 12, 2015

Contributor

that's done by using the keys in the ctor?

@s1monw

s1monw Aug 12, 2015

Contributor

that's done by using the keys in the ctor?

This comment has been minimized.

@rjernst

rjernst Aug 12, 2015

Member

Ah yes, thanks!

@rjernst

rjernst Aug 12, 2015

Member

Ah yes, thanks!

@rjernst

View changes

Show outdated Hide outdated core/src/test/java/org/elasticsearch/common/inject/ModuleTestCase.java Outdated
@rjernst

View changes

Show outdated Hide outdated core/src/test/java/org/elasticsearch/common/inject/ModuleTestCase.java Outdated
@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Aug 12, 2015

Member

This looks great in general, I think it will simplify a lot where we have extension points, so we get the same logic/exception checking. I left a few comments/questions.

Member

rjernst commented Aug 12, 2015

This looks great in general, I think it will simplify a lot where we have extension points, so we get the same logic/exception checking. I left a few comments/questions.

@rjernst

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/common/util/ExtensionPoint.java Outdated
@rjernst

View changes

Show outdated Hide outdated core/src/main/java/org/elasticsearch/common/util/ExtensionPoint.java Outdated
@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Aug 12, 2015

Member

LGTM. I left two more questions but those can be follow ups or ignored.

Member

rjernst commented Aug 12, 2015

LGTM. I left two more questions but those can be follow ups or ignored.

Introduce a formal ExtensionPoint class to stream line extensions
This commit tries to add some infrastructure to streamline how extension
points should be strucutred. It's a simple approache with 4 implementations
for `highlighter`, `suggester`, `allocation_decider` and `shards_allocator`.
It simplifies adding new extension points and forces to register classes instead
of strings.

@s1monw s1monw merged commit 74f18d8 into elastic:master Aug 13, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details
* all extensions by a single name and ensures that extensions are not registered
* more than once.
*/
public abstract class ExtensionPoint<T> {

This comment has been minimized.

@nik9000

nik9000 Aug 13, 2015

Contributor

I like these as they wrap guice ceremony.

@nik9000

nik9000 Aug 13, 2015

Contributor

I like these as they wrap guice ceremony.

@@ -182,12 +180,12 @@ public void registerStream(MovAvgModelStreams.Stream stream) {
MovAvgModelStreams.registerStream(stream);
}
public void registerHighlighter(Class<? extends Highlighter> clazz) {
highlighters.add(clazz);
public void registerHighlighter(String key, Class<? extends Highlighter> clazz) {

This comment has been minimized.

@nik9000

nik9000 Aug 13, 2015

Contributor

Can we do away with these entirely and just let plugins work with instances of ExtensionPoint?

@nik9000

nik9000 Aug 13, 2015

Contributor

Can we do away with these entirely and just let plugins work with instances of ExtensionPoint?

@@ -245,14 +243,7 @@ protected void configureFetchSubPhase() {
}
protected void configureSuggesters() {

This comment has been minimized.

@nik9000

nik9000 Aug 13, 2015

Contributor

This whole method is now only useful for backwards compatibility. I don't really think this is needed any more.

@nik9000

nik9000 Aug 13, 2015

Contributor

This whole method is now only useful for backwards compatibility. I don't really think this is needed any more.

}
private Highlighters(Map<String, Highlighter> parsers) {
super("highlighter", Highlighter.class, new HashSet<>(Arrays.asList(FVH, FAST_VECTOR_HIGHLIGHTER, PLAIN, HIGHLIGHTER, POSTINGS, POSTINGS_HIGHLIGHTER)),

This comment has been minimized.

@nik9000

nik9000 Aug 13, 2015

Contributor

I mean, it'd be super rude for an extension to relace a builtin highlighter but..... why should we be so careful to stop them?

@nik9000

nik9000 Aug 13, 2015

Contributor

I mean, it'd be super rude for an extension to relace a builtin highlighter but..... why should we be so careful to stop them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment