-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Example stable plugins with settings #92334
Conversation
Hi @pgomulka, I've created a changelog YAML for you. |
Pinging @elastic/es-core-infra (Team:Core/Infra) |
@elasticmachine ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. My general suggestion is to document more. IMO the examples need to be completely self documenting, and a plugin developer looking at example code likely doesn't know much, if anything, about the Lucene APIs, so I would be as explicit as you can in explaining why certain interfaces or methods are overridden, and what the example plugin is supposed to do.
@@ -1,4 +1,4 @@ | |||
apply plugin: 'elasticsearch.stable-esplugin' | |||
apply plugin: 'elasticsearch.stable-esplugin' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unintentional indent?
import java.util.List; | ||
|
||
@AnalysisSettings | ||
public interface ExampleAnalysisSettings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful to have javadocs on all of the classes and methods in these examples, so that developers can read through and understand the purpose and function of each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there should be javadocs pointing to how these methods are used in examples. will add.
however I am unsure on the naming approach for this code.
This example plugin does not really do anything sensible. It is just meant to show the mechanics of how the new stable plugin api can be used.
so for instance I named ExampleCharFilterFactory
but I could name it ReplaceCharWithNumberCharFilterFactory
. In a way the latter describes what that class does, but is a reader of this code really interest about the "fake business logic" here? I wonder if it wouldn't actually make the code harder to navigate and understand.
therfore I feel tempted to stay with the current Example*Factory, Custom* etc style of naming.
I chatted with @rjernst about this and we couldn't decide. What's your view on this @williamrandolph ? If we were to point to this code from the guide, which naming style would be better to you?
|
||
public class UnderscoreTokenizer extends CharTokenizer { | ||
public class CharTokenizer extends org.apache.lucene.analysis.util.CharTokenizer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use another name that doesn't cause a need for the fully qualified superclass? Even CustomCharTokenizer would be better than naming the class the same as it's superclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use CustomCharTokenizer
@@ -14,16 +14,18 @@ | |||
|
|||
import java.io.IOException; | |||
|
|||
public class Skip1TokenFilter extends FilteringTokenFilter { | |||
public class SkipTokenFilter extends FilteringTokenFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this name more descriptive? Isn't this specifically skipping a certain ascii digit?
server/src/main/resources/org/elasticsearch/bootstrap/security.policy
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, no need for further review if you agree with all the comments.
@@ -22,6 +22,7 @@ dependencies { | |||
testImplementation ('junit:junit:4.13.2'){ | |||
exclude group: 'org.hamcrest' | |||
} | |||
testImplementation ('org.mockito:mockito-core:4.4.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no parenthesis needed
|
||
@Override | ||
protected TokenStreamComponents createComponents(String fieldName) { | ||
var tokenizerListOfChars = settings.singleCharsToSkipInTokenizer().isEmpty() ? List.of("_") : settings.singleCharsToSkipInTokenizer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does ListSetting not have a definable default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does not. it returns an empty list.
but this is good point, perhaps we should have a default. I will create an issue to follow up. this might be problem when defining a default for a list of custom types.
|
||
public CharSkippingTokenizer(List<String> tokenizerListOfChars) { | ||
this.setOfChars = tokenizerListOfChars.stream().map(s -> (int) s.charAt(0)).collect(Collectors.toSet()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove empty line
|
||
/** | ||
* Implementation notes to all components: | ||
* - a @NamedComponent annotation with a name is required in order for the component to be found by Elasticsearch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a real javadoc list? ie use <ul>...
* - a constructor is annotated with @Inject and has a settings interface as an argument. See the javadoc for the | ||
* ExampleAnalysisSettings for more details | ||
* - a no/noarg constructor is also possible | ||
* - a methods from stable analysis api are to be implemented with apache lucene |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: capitalize Apache Lucene
|
||
import org.apache.lucene.analysis.FilteringTokenFilter; | ||
import org.apache.lucene.analysis.TokenStream; | ||
import org.apache.lucene.analysis.tokenattributes.CharTermAttribute; | ||
|
||
import java.io.IOException; | ||
|
||
public class Skip1TokenFilter extends FilteringTokenFilter { | ||
public class SkipAsciiDigits extends FilteringTokenFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this class moved into server, shouldn't it be in the example stable analysis plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks confusing because the same classes are used in server for unit testing.
Skip1TokenFilter
in examples was replaced with SkipStartingWithDigitTokenFilter
I will rename the classes in server to reflect those in examples.
@@ -219,7 +219,7 @@ static class CustomAnalyzer extends Analyzer { | |||
protected TokenStreamComponents createComponents(String fieldName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this class be in the example stable analysis plugin?
New stable plugins example with injected settings. Plugin developer creates an interface and annotates it with @AnalysisSettings. The constructor in a plugin component (annotated with @NamedComponent) has to be annotated with @Inject Upon plugin component creation an implementation of the interface will be created - a dynamic proxy - which will delegate methods from interface to properties in a Settings instance. This PR introduces an example of using all currently supported types : int, long, double, string and list (of strings) relates #elastic#88980
I accidentally added a parameter to ListSetting #92334. this commit removes it
New stable plugins example with injected settings.
Plugin developer creates an interface and annotates it with @AnalysisSettings.
The constructor in a plugin component (annotated with @NamedComponent) has to be annotated with @Inject
Upon plugin component creation an implementation of the interface will be created - a dynamic proxy - which will delegate methods from interface to properties in a Settings instance.
This PR introduces an example of using all currently supported types : int, long, double, string and list (of strings)
relates ##88980