Consolidate all Plugin.createComponents arguments into a single accessor class#101305
Consolidate all Plugin.createComponents arguments into a single accessor class#101305thecoop merged 3 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| * | ||
| * @deprecated New services will only be added to {@link PluginServices} | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
This can just be left around for compatibility, but any new services would be added to PluginServices only
There was a problem hiding this comment.
We don't need to do this, or at least we can remove this once serverless is updated. There are no compatibility guarantees for non-stable plugins.
There was a problem hiding this comment.
I'll migrate all the existing implementations as a separate PR
| indicesService | ||
| ) | ||
| ).toList(); | ||
| Plugin.PluginServices services = new Plugin.PluginServices() { |
There was a problem hiding this comment.
I think this is OK but a bit verbose; maybe a builder would be nicer to use
Plugin.ComponentsBuilder b = p.createComponents();
var componetns = b
.withClient(client)
.withClusterService(clusterService)
...
.build();
or
var components = p.createComponents(b -> b.withClient(client).withClusterService(clusterService)...)
Just suggesting alternatives, it's OK either way
There was a problem hiding this comment.
A builder is good when you're building something up in stages, but this is all-or-nothing - you need all these things to have a PluginServices object, it makes no sense without them. It'll also add a lot of boilerplate to the builder implementation & verification.
There was a problem hiding this comment.
👍 but in terms of boilerplate-reduction why not just use a record? Do we really need an interface and an anonymous class to implement it?
There was a problem hiding this comment.
I considered that...an interface gives us a bit more flexibility for the future, and this is a formal public class contract so should be done more 'properly'
There was a problem hiding this comment.
A record seems more suitable to me. What flexibility to you envision an interface gives? What makes it a better formal contract?
There was a problem hiding this comment.
I would say an interface is more usual in an API like this. It can be mocked out easily, and it allows flexibility for any future changes we may wish to add, such as, off the top of my head:
- Adding code to track which services are accessed by which plugins
- Exceptions can be thrown if a service is not available in a particular context
- Dynamically loading services into particular class contexts only when they are accessed
- Test implementations can add restrictions on how services are accessed, or check they are only accessed in certain threads
- We could migrate to a more listener/registration based API using this interface as a base, maybe moving some of the existing methods/plugin interfaces into here in the future
- We could split this interface up into different sub-interfaces for different types of plugins
There's a whole load of future modifications we could do by using an interface. A record doesn't allow these. And there's nothing additional a record gives us here that an interface doesn't.
There was a problem hiding this comment.
I've changed the implementing class to a record, so it's a bit more succinct in the definition, and not anonymous
Create a
createComponentsoverload that only takes a single services argument, allowing new services to be added without breaking API compatibility as much.Keep the old method for compatibility with existing plugins