Deguice rest handlers #22575

nik9000 · 2017-01-12T00:45:51Z

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.

nik9000 · 2017-01-12T00:48:24Z

core/src/main/java/org/elasticsearch/action/ActionModule.java

        this.transportClient = transportClient;
        this.settings = settings;
+        this.indexNameExpressionResolver = indexNameExpressionResolver;
+        this.indexScopedSettings = indexScopedSettings;
+        this.clusterSettings = clusterSettings;


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.

nik9000 · 2017-01-12T00:48:52Z

...client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/NoopPlugin.java

@@ -43,7 +51,11 @@
    }

    @Override
-    public List<Class<? extends RestHandler>> getRestHandlers() {
-        return Arrays.asList(RestNoopBulkAction.class, RestNoopSearchAction.class);
+    public List<RestHandler> initRestHandlers(Settings settings, RestController restController, ClusterSettings clusterSettings,


See commit message for my I did this instead of some fancy ctor reference stuff.

nik9000 · 2017-01-12T00:49:05Z

...p-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java

@@ -47,7 +46,6 @@
 import static org.elasticsearch.rest.RestStatus.OK;

 public class RestNoopBulkAction extends BaseRestHandler {
-    @Inject


Most of the files I touched I did this.

nik9000 · 2017-01-12T00:49:33Z

core/src/main/java/org/elasticsearch/common/path/PathTrie.java

@@ -19,6 +19,8 @@

 package org.elasticsearch.common.path;

+import joptsimple.internal.Strings;


Oops. I'll remove.

nik9000 · 2017-01-12T00:49:46Z

core/src/main/java/org/elasticsearch/common/path/PathTrie.java

@@ -111,7 +113,10 @@ public synchronized void insert(String[] path, int index, T value) {
                // in case the target(last) node already exist but without a value
                // than the value should be updated.
                if (index == (path.length - 1)) {
-                    assert (node.value == null || node.value == value);
+                    if (node.value != null) {
+                        throw new IllegalArgumentException("Path [" + Strings.join(path, "/")+ "] already has a value ["


Since we lost the "no duplicate" behavior from ActionModule I figured we should duplicate it here, but with paths.

nik9000 · 2017-01-12T00:50:29Z

core/src/main/java/org/elasticsearch/common/settings/SettingsModule.java

@@ -218,4 +220,8 @@ public IndexScopedSettings getIndexScopedSettings() {
    public ClusterSettings getClusterSettings() {
        return clusterSettings;
    }
+
+    public SettingsFilter getSettingsFilter() {


Exposed to ActionModule to build rest handlers. Still needs to be bound I think.

nik9000 · 2017-01-12T00:50:47Z

core/src/main/java/org/elasticsearch/plugins/ActionPlugin.java

        return Collections.emptyList();
    }

+


Oops. No need for this one. Can remove.

nik9000 · 2017-01-12T00:51:27Z

core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestCancelTasksAction.java


 import static org.elasticsearch.rest.RestRequest.Method.POST;
 import static org.elasticsearch.rest.action.admin.cluster.RestListTasksAction.listTasksResponseListener;


 public class RestCancelTasksAction extends BaseRestHandler {
-    private final ClusterService clusterService;
+    private final Supplier<DiscoveryNodes> nodesInCluster;


I switched this to the provider because I thought it'd be cleaner than exposing all of ClusterService.

nik9000 · 2017-01-12T00:52:22Z

core/src/test/java/org/elasticsearch/action/ActionModuleTests.java

@@ -99,32 +109,69 @@ public ActionResponse newResponse() {
    }

    public void testSetupRestHandlerContainsKnownBuiltin() {
-        assertThat(ActionModule.setupRestHandlers(emptyList()), hasItem(RestMainAction.class));
+        SettingsModule settings = new SettingsModule(Settings.EMPTY);


I switched these to assert based on paths instead of class which makes more sense in the new world.

nik9000 · 2017-01-12T00:57:58Z

core/src/main/java/org/elasticsearch/action/ActionModule.java

        for (ActionPlugin plugin : actionPlugins) {
-            for (Class<? extends RestHandler> handler : plugin.getRestHandlers()) {
-                registerRestHandler(handlers, handler);
+            for (RestHandler handler : plugin.initRestHandlers(settings, restController, clusterSettings, indexScopedSettings,


I wonder if instead of exposing all of these to plugins we should remove a few. Some (indexScopedSettings, clusterSettings) are fairly single purpose.

rjernst · 2017-01-12T08:39:45Z

core/src/main/java/org/elasticsearch/plugins/ActionPlugin.java

@@ -63,7 +72,9 @@
    /**
     * Rest handlers added by this plugin.
     */
-    default List<Class<? extends RestHandler>> getRestHandlers() {
+    default List<RestHandler> initRestHandlers(Settings settings, RestController restController, ClusterSettings clusterSettings,


I think we should still call this getRestHandlers

nik9000 · 2017-01-17T20:49:11Z

@rjernst, could you have another look?

rjernst

LGTM, thanks for this!

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.

nik9000 · 2017-01-20T16:49:00Z

Thanks for reviewing @rjernst!

nik9000 · 2017-01-20T16:49:41Z

master: 6265ef1
5.x: 6050f22

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) ...

nik9000 added :Core/Infra/Plugins Plugin API and infrastructure >breaking-java review v5.3.0 v6.0.0-alpha1 labels Jan 12, 2017

nik9000 requested a review from rjernst January 12, 2017 00:46

nik9000 commented Jan 12, 2017

View reviewed changes

nik9000 force-pushed the deguice_rest_handlers branch from 7aeccca to c7f23f1 Compare January 12, 2017 00:53

nik9000 commented Jan 12, 2017

View reviewed changes

rjernst reviewed Jan 12, 2017

View reviewed changes

rjernst approved these changes Jan 17, 2017

View reviewed changes

nik9000 force-pushed the deguice_rest_handlers branch from 8a9a5f4 to 1b0bb4c Compare January 20, 2017 15:47

nik9000 added 5 commits January 20, 2017 11:47

Replace silly method

9a79b80

Spacing fix

7d9587c

Call it getRestHandlers

50c5eb5

Fix from rebase

a7cd47a

nik9000 force-pushed the deguice_rest_handlers branch from 1b0bb4c to a7cd47a Compare January 20, 2017 16:47

nik9000 merged commit 6265ef1 into elastic:master Jan 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deguice rest handlers #22575

Deguice rest handlers #22575

nik9000 commented Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 Jan 12, 2017

rjernst Jan 12, 2017

nik9000 Jan 12, 2017

nik9000 commented Jan 17, 2017

rjernst left a comment

nik9000 commented Jan 20, 2017

nik9000 commented Jan 20, 2017 •

edited

		@@ -19,6 +19,8 @@

		package org.elasticsearch.common.path;

		import joptsimple.internal.Strings;

Deguice rest handlers #22575

Deguice rest handlers #22575

Conversation

nik9000 commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Jan 17, 2017

rjernst left a comment

Choose a reason for hiding this comment

nik9000 commented Jan 20, 2017

nik9000 commented Jan 20, 2017 • edited

nik9000 commented Jan 20, 2017 •

edited