Example stable plugins with settings by pgomulka · Pull Request #92334 · elastic/elasticsearch

pgomulka · 2022-12-13T19:01:33Z

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

…in_api_settings

…ings

elasticsearchmachine · 2022-12-13T19:01:57Z

Hi @pgomulka, I've created a changelog YAML for you.

elasticsearchmachine · 2022-12-13T19:01:57Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

…ings

…example_plugin_settings

pgomulka · 2022-12-14T17:03:18Z

@elasticmachine ok to test

…ings

rjernst

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.

rjernst · 2022-12-19T20:49:13Z

plugins/examples/stable-analysis/build.gradle

@@ -1,4 +1,4 @@
-apply plugin: 'elasticsearch.stable-esplugin'
+  apply plugin: 'elasticsearch.stable-esplugin'


unintentional indent?

rjernst · 2022-12-19T21:01:15Z

...table-analysis/src/main/java/org/elasticsearch/example/analysis/ExampleAnalysisSettings.java

+import java.util.List;
+
+@AnalysisSettings
+public interface ExampleAnalysisSettings {


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.

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?

rjernst · 2022-12-20T00:01:04Z

...s/stable-analysis/src/main/java/org/elasticsearch/example/analysis/lucene/CharTokenizer.java

+import java.util.stream.Collectors;

-public class UnderscoreTokenizer extends CharTokenizer {
+public class CharTokenizer extends org.apache.lucene.analysis.util.CharTokenizer {


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.

I will use CustomCharTokenizer

rjernst · 2022-12-20T00:06:24Z

...stable-analysis/src/main/java/org/elasticsearch/example/analysis/lucene/SkipTokenFilter.java

 import java.io.IOException;

-public class Skip1TokenFilter extends FilteringTokenFilter {
+public class SkipTokenFilter extends FilteringTokenFilter {


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

rjernst

LGTM, no need for further review if you agree with all the comments.

rjernst · 2023-01-10T14:20:19Z

plugins/examples/stable-analysis/build.gradle

  testImplementation ('junit:junit:4.13.2'){
    exclude group: 'org.hamcrest'
  }
+  testImplementation ('org.mockito:mockito-core:4.4.0')


no parenthesis needed

rjernst · 2023-01-10T14:22:15Z

.../stable-analysis/src/main/java/org/elasticsearch/example/analysis/CustomAnalyzerFactory.java

+
+        @Override
+        protected TokenStreamComponents createComponents(String fieldName) {
+            var tokenizerListOfChars = settings.singleCharsToSkipInTokenizer().isEmpty() ? List.of("_") : settings.singleCharsToSkipInTokenizer();


does ListSetting not have a definable default?

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.

rjernst · 2023-01-10T14:24:12Z

...-analysis/src/main/java/org/elasticsearch/example/analysis/lucene/CharSkippingTokenizer.java

+
+    public CharSkippingTokenizer(List<String> tokenizerListOfChars) {
+        this.setOfChars = tokenizerListOfChars.stream().map(s -> (int) s.charAt(0)).collect(Collectors.toSet());
+


nit: remove empty line

rjernst · 2023-01-10T14:25:32Z

.../examples/stable-analysis/src/main/java/org/elasticsearch/example/analysis/package-info.java

+
+/**
+ * Implementation notes to all components:
+ * - a @NamedComponent annotation with a name is required in order for the component to be found by Elasticsearch


Make this a real javadoc list? ie use <ul>...

rjernst · 2023-01-10T14:26:20Z

.../examples/stable-analysis/src/main/java/org/elasticsearch/example/analysis/package-info.java

+ * - 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


nit: capitalize Apache Lucene

rjernst · 2023-01-10T14:27:39Z

server/src/test/java/org/elasticsearch/indices/analysis/lucene/SkipAsciiDigits.java

 import java.io.IOException;

-public class Skip1TokenFilter extends FilteringTokenFilter {
+public class SkipAsciiDigits extends FilteringTokenFilter {


why is this class moved into server, shouldn't it be in the example stable analysis plugin?

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.

rjernst · 2023-01-10T14:28:40Z

...src/test/java/org/elasticsearch/indices/analysis/StableAnalysisPluginsWithSettingsTests.java

@@ -219,7 +219,7 @@ static class CustomAnalyzer extends Analyzer {
            protected TokenStreamComponents createComponents(String fieldName) {


Shouldn't this class be in the example stable analysis plugin?

…ings

@Inject

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

pgomulka added 20 commits November 9, 2022 15:33

Stable Plugin Api settings

d888345

javadoc

2e6a55a

Update docs/changelog/91467.yaml

7c8ec5d

inject rename

3a0edde

remove max until validation is implemented

8e295d4

Merge remote-tracking branch 'pgomulka/plugin_api_settings' into plug…

b311aae

…in_api_settings

char filter serach and config

df65b58

token filter

07d7234

tokenizer tests

bd3f109

analyzer test

5b57b85

Merge remote-tracking branch 'upstream/main' into example_plugin_sett…

67dbcbe

…ings

rename list of strings to List

f5369ef

Merge remote-tracking branch 'upstream/main' into plugin_api_settings

edc3541

Merge remote-tracking branch 'upstream/main' into plugin_api_settings

fe9eb20

Merge remote-tracking branch 'upstream/main' into plugin_api_settings

ddd06d1

rename injectsettings to inject

a8628dc

precommit

9a8bf3f

Merge branch 'plugin_api_settings' into example_plugin_settings

01b8c14

refactor to latest settings api

b3ab33f

Merge remote-tracking branch 'upstream/main' into example_plugin_sett…

71f90f4

…ings

pgomulka added >enhancement :Core/Infra/Plugins Plugin API and infrastructure v8.7.0 labels Dec 13, 2022

pgomulka self-assigned this Dec 13, 2022

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Dec 13, 2022

Update docs/changelog/92334.yaml

d157ae6

pgomulka mentioned this pull request Dec 13, 2022

Stable Analysis Plugin API #88980

Open

25 tasks

security policy for reflect

e6d0673

pgomulka added 3 commits December 14, 2022 12:51

Merge remote-tracking branch 'upstream/main' into example_plugin_sett…

b918ca7

…ings

Merge remote-tracking branch 'pgomulka/example_plugin_settings' into …

07a1eda

…example_plugin_settings

list of strings'

e7c3b01

pgomulka added 3 commits December 15, 2022 17:59

remove debug

36b95e7

Merge remote-tracking branch 'upstream/main' into example_plugin_sett…

f2cb282

…ings

fix npe

cedae0e

rjernst reviewed Dec 20, 2022

View reviewed changes

pgomulka added 3 commits December 20, 2022 19:18

code review folow up

d0b08aa

javadocs

9994d9e

rename classes in examples

0126e8d

pgomulka requested a review from rjernst January 9, 2023 18:02

rjernst approved these changes Jan 10, 2023

View reviewed changes

pgomulka added 3 commits January 11, 2023 15:11

code review follow up

974c33a

more renames

67976c6

Merge remote-tracking branch 'upstream/main' into example_plugin_sett…

117dcde

…ings

pgomulka merged commit 742c6f3 into elastic:main Jan 11, 2023

This was referenced Jan 12, 2023

[Stable plugin api] Add a default parameter to ListSetting #92874

Open

Remove accidentaly added parameter to ListSetting #92875

Merged

pgomulka added a commit that referenced this pull request Jan 12, 2023

Remove accidentaly added parameter to ListSetting (#92875)

e102edf

I accidentally added a parameter to ListSetting #92334. this commit removes it

ChrisHegarty unassigned pgomulka Oct 29, 2024

		@@ -1,4 +1,4 @@
		apply plugin: 'elasticsearch.stable-esplugin'
		apply plugin: 'elasticsearch.stable-esplugin'


		public CharSkippingTokenizer(List<String> tokenizerListOfChars) {
		this.setOfChars = tokenizerListOfChars.stream().map(s -> (int) s.charAt(0)).collect(Collectors.toSet());

		@@ -219,7 +219,7 @@ static class CustomAnalyzer extends Analyzer {
		protected TokenStreamComponents createComponents(String fieldName) {

Conversation

pgomulka commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 13, 2022

Uh oh!

elasticsearchmachine commented Dec 13, 2022

Uh oh!

pgomulka commented Dec 14, 2022

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pgomulka commented Dec 13, 2022 •

edited

Loading

pgomulka Dec 20, 2022 •

edited

Loading