-
Notifications
You must be signed in to change notification settings - Fork 0
Support adding to injector instances from fields #6
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
base: di/spacetime
Are you sure you want to change the base?
Support adding to injector instances from fields #6
Conversation
# Conflicts: # libs/plugin-scanner/src/main/java/org/elasticsearch/plugin/scanner/ManifestBuilder.java # server/src/main/java/org/elasticsearch/plugins/BundleManifest.java
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.
Looks fine, couple nits
|
||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(value = { TYPE, FIELD }) | ||
public @interface Extension { |
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.
wdyt of calling this "Constant"? Extension is very close to the existing Extensible...
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.
Maybe @instance or @InjectableInstance?
return List.of(downsample); | ||
} | ||
@Extension | ||
public static final FixedExecutorBuilderSpec DOWNSAMPLE_EXECUTOR = new FixedExecutorBuilderSpec( |
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 we want this where it is used so the Plugin subclass can be removed. I believe that is in the downsample persistent task executor?
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.
Yes, I left it here for now just to make the change easier to understand, but I can move it
This PR addresses the following situation: we have a core component that injects a list of other components:
Where A is extensible:
These A components don't add any additional logic but simply provide configuration.
So instead of requiring them to extend A, the preferred approach is to create instances of A that can be registered with the injector:
Let's look at this with a concrete example:
ThreadPool
is service, which injects a collection ofFixedExecutorBuilderSpec
(only fixed for now) (in the future they should replaceExecutorBuilder
)FixedExecutorBuilderSpec
provides all necessary information to create executor that will be used inThreadPool
.FixedExecutorBuilderSpec
is marked with annotation@Extensible
In Downsampling plugin we create an instance of
FixedExecutorBuilderSpec
and mark it with annotation@Extension
: