Conversation
There was a problem hiding this comment.
I'm not really sure if it'd have been better to have the bits that are only needed to init the rest handlers on the rest handler method instead of the ctor but I went with the ctor because it felt like how we build these module now. I'm happy either way.
There was a problem hiding this comment.
See commit message for my I did this instead of some fancy ctor reference stuff.
There was a problem hiding this comment.
Most of the files I touched I did this.
There was a problem hiding this comment.
Since we lost the "no duplicate" behavior from ActionModule I figured we should duplicate it here, but with paths.
There was a problem hiding this comment.
Exposed to ActionModule to build rest handlers. Still needs to be bound I think.
There was a problem hiding this comment.
Oops. No need for this one. Can remove.
There was a problem hiding this comment.
I switched this to the provider because I thought it'd be cleaner than exposing all of ClusterService.
There was a problem hiding this comment.
I switched these to assert based on paths instead of class which makes more sense in the new world.
7aeccca to
c7f23f1
Compare
There was a problem hiding this comment.
I wonder if instead of exposing all of these to plugins we should remove a few. Some (indexScopedSettings, clusterSettings) are fairly single purpose.
There was a problem hiding this comment.
I think we should still call this getRestHandlers
|
@rjernst, could you have another look? |
8a9a5f4 to
1b0bb4c
Compare
There are presently 7 ctor args used in any rest handlers: * `Settings`: Every handler uses it to initialize a logger and some other strange things. * `RestController`: Every handler registers itself with it. * `ClusterSettings`: Used by `RestClusterGetSettingsAction` to render the default values for cluster settings. * `IndexScopedSettings`: Used by `RestGetSettingsAction` to get the default values for index settings. * `SettingsFilter`: Used by a few handlers to filter returned settings so we don't expose stuff like passwords. * `IndexNameExpressionResolver`: Used by `_cat/indices` to filter the list of indices. * `Supplier<DiscoveryNodes>`: Used to fill enrich the response by handlers that list tasks. We probably want to reduce these arguments over time but switching construction away from guice gives us tighter control over the list of available arguments. These parameters are passed to plugins using `ActionPlugin#initRestHandlers` which is expected to build and return that handlers immediately. This felt simpler than returning an reference to the ctors given all the different possible args.
1b0bb4c to
a7cd47a
Compare
|
Thanks for reviewing @rjernst! |
There are presently 7 ctor args used in any rest handlers: * `Settings`: Every handler uses it to initialize a logger and some other strange things. * `RestController`: Every handler registers itself with it. * `ClusterSettings`: Used by `RestClusterGetSettingsAction` to render the default values for cluster settings. * `IndexScopedSettings`: Used by `RestGetSettingsAction` to get the default values for index settings. * `SettingsFilter`: Used by a few handlers to filter returned settings so we don't expose stuff like passwords. * `IndexNameExpressionResolver`: Used by `_cat/indices` to filter the list of indices. * `Supplier<DiscoveryNodes>`: Used to fill enrich the response by handlers that list tasks. We probably want to reduce these arguments over time but switching construction away from guice gives us tighter control over the list of available arguments. These parameters are passed to plugins using `ActionPlugin#initRestHandlers` which is expected to build and return that handlers immediately. This felt simpler than returning an reference to the ctors given all the different possible args. Breaks java plugins by moving rest handlers off of guice.
* master: (33 commits) Docs fix - Added missing link to new Adjacency-matrix agg Pass `forceExecution` flag to transport interceptor (elastic#22739) Version: Add missing releases from 2.x in Version.java (elastic#22594) CONSOLE-ify filter aggregation docs CONSOLE-ify date_range aggregation docs Add single static instance of SpecialPermission (elastic#22726) Simplify InternalEngine#innerIndex (elastic#22721) Upgrade to Lucene 6.4.0 (elastic#22724) Fix broken TaskInfo.toString() Add CheckedSupplier and CheckedRunnable to core (elastic#22725) Revert "Make build Gradle 2.14 / 3.x compatible (elastic#22669)" Fixes retrieval of the latest snapshot index blob (elastic#22700) CONSOLE-ify date histogram docs CONSOLE-ify min and max aggregation docs CONSOLE-ify global-aggregation.asciidoc Fix script score function that combines _score and weight (elastic#22713) Corrected a plural verb to a singular one. (elastic#22681) Fix duplicates from search.query (elastic#22701) Readd unconverted snippets mark for doc Deguice rest handlers (elastic#22575) ...
There are presently 7 ctor args used in any rest handlers:
Settings: Every handler uses it to initialize a logger andsome other strange things.
RestController: Every handler registers itself with it.ClusterSettings: Used byRestClusterGetSettingsActiontorender the default values for cluster settings.
IndexScopedSettings: Used byRestGetSettingsActionto getthe default values for index settings.
SettingsFilter: Used by a few handlers to filter returnedsettings so we don't expose stuff like passwords.
IndexNameExpressionResolver: Used by_cat/indicestofilter the list of indices.
Supplier<DiscoveryNodes>: Used to fill enrich the responseby handlers that list tasks.
We probably want to reduce these arguments over time but
switching construction away from guice gives us tighter
control over the list of available arguments.
These parameters are passed to plugins using
ActionPlugin#initRestHandlerswhich is expected to build andreturn that handlers immediately. This felt simpler than
returning an reference to the ctors given all the different
possible args.
Breaks java plugins by moving rest handlers off of guice.