Add Cluster Put Settings API to the high level REST client #28633

olcbean · 2018-02-12T12:39:01Z

Add Cluster Put Settings API to the high level REST client
( equivalent to Cluster Update Setting in transport client )

Relates to #27205

elasticmachine · 2018-02-12T12:39:03Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-02-12T12:39:03Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

olcbean · 2018-02-12T12:46:31Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ClusterClient.java

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-update-settings.html"> Cluster Update Settings
+     * API on elastic.co</a>
+     */
+    public ClusterUpdateSettingsResponse updateSettings(ClusterUpdateSettingsRequest clusterUpdateSettingsRequest, Header... headers)


hey @javanna I need some help with the naming?
In the rest-api-specs the api is cluster.put_settings, in the transport client and in the issue it is updateSettings. Should I rename to putSettings or leave it as updateSettings?

let's follow the SPEC and make it putSettings please ;)

javanna

left some comments, looks good though, thanks @olcbean !

javanna · 2018-02-13T10:20:43Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ClusterClient.java

+/**
+ * A wrapper for the {@link RestHighLevelClient} that provides methods for accessing the cluster api.
+ * <p>
+ * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster.html">Indices API on elastic.co</a>


s/Indices API/Cluster API

javanna · 2018-02-13T10:22:33Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ClusterClient.java

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-update-settings.html"> Cluster Update Settings
+     * API on elastic.co</a>
+     */
+    public ClusterUpdateSettingsResponse updateSettings(ClusterUpdateSettingsRequest clusterUpdateSettingsRequest, Header... headers)


let's follow the SPEC and make it putSettings please ;)

javanna · 2018-02-13T10:26:47Z

client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java

+        Map<String, Object> setMap = getAsMap("/_cluster/settings");
+        String transietSetValue = (String) XContentMapValues.extractValue("transient." + transientSettingKey, setMap);
+        assertThat(transietSetValue, equalTo(transientSettingValue + ByteSizeUnit.BYTES.getSuffix()));
+        String persistentSetValue = (String) XContentMapValues.extractValue("persistent." + persistentSettingKey, setMap);


s/transiet/transient

javanna · 2018-02-13T10:27:14Z

client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java

+        assertThat(resetResponse.getPersistentSettings(), equalTo(Settings.EMPTY));
+
+        Map<String, Object> resetMap = getAsMap("/_cluster/settings");
+        String transietResetValue = (String) XContentMapValues.extractValue("transient." + transientSettingKey, resetMap);


s/transiet/transient

javanna · 2018-02-13T10:28:30Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java

-            if (randomBoolean()) {
-                setRandomWaitForActiveShards(resizeRequest::setWaitForActiveShards, expectedParams);
-            }
+            setRandomWaitForActiveShards(resizeRequest::setWaitForActiveShards, expectedParams);


why these two changes? I meant to to randomly choose one of the two branches, but then still set the param only randomly.

if (randomBoolean()) is already wrapped in the setRandom*. If we keep it here, then waitForRandomShards is set to a random value only 25% of the time.
I assumed that it was an oversight...

private static void setRandomWaitForActiveShards(Consumer<ActiveShardCount> setter, Map<String, String> expectedParams) { if (randomBoolean()) { String waitForActiveShardsString; int waitForActiveShards = randomIntBetween(-1, 5); if (waitForActiveShards == -1) { waitForActiveShardsString = "all"; } else { waitForActiveShardsString = String.valueOf(waitForActiveShards); } setter.accept(ActiveShardCount.parseString(waitForActiveShardsString)); expectedParams.put("wait_for_active_shards", waitForActiveShardsString); } }

oh right :) thanks

javanna · 2018-02-13T10:45:59Z

...main/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsResponse.java


 /**
 * A response for a cluster update settings action.
 */
-public class ClusterUpdateSettingsResponse extends AcknowledgedResponse {
+public class ClusterUpdateSettingsResponse extends AcknowledgedResponse implements ToXContentObject {


shall we move RestClusterUpdateSettingsAction to use RestToXContentListener?

javanna · 2018-02-13T10:47:43Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

@@ -0,0 +1,128 @@
+[[java-rest-high-cluster-update-settings]]


rename the file to put_settings?

javanna · 2018-02-13T10:48:26Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

+provided as an argument
+<2> Called in case of a failure. The raised exception is provided as an argument
+
+[[java-rest-high-clustre-update-settings-response]]


s/clustre/cluster

javanna · 2018-02-13T10:54:13Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

+--------------------------------------------------
+<1> Indicates whether all of the nodes have acknowledged the request
+<2> Indicates which transient settings have been applied
+<3> Indicates which persistent settings have been applied


the docs don't quite build, could you fix that up?

javanna · 2018-02-13T10:56:43Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

+--------------------------------------------------
+include-tagged::{doc-tests}/ClusterClientDocumentationIT.java[update-settings-settings-map]
+--------------------------------------------------
+<1> Settings provided as `Object` key-pairs, which gets converted to


you mean as a map?

tlrx · 2018-02-13T10:19:07Z

client/rest-high-level/src/main/java/org/elasticsearch/client/ClusterClient.java

+import static java.util.Collections.emptySet;
+
+/**
+ * A wrapper for the {@link RestHighLevelClient} that provides methods for accessing the cluster api.


cluster api -> Cluster API

tlrx · 2018-02-13T10:25:37Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

@@ -240,6 +241,15 @@ public final IndicesClient indices() {
        return indicesClient;
    }

+    /**
+     * Provides a {@link ClusterClient} which can be used to access the Clustre API.


Fix Clustre

tlrx · 2018-02-13T11:03:28Z

client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java

+        String persistentSetValue = (String) XContentMapValues.extractValue("persistent." + persistentSettingKey, setMap);
+        assertThat(persistentSetValue, equalTo(persistentSettingValue));
+
+        ClusterUpdateSettingsRequest reserRequest = new ClusterUpdateSettingsRequest();


resetRequest

tlrx · 2018-02-13T11:06:42Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+
+        Settings transientSettings = Settings.builder().put(transientSettingKey, transientSettingValue, ByteSizeUnit.BYTES).build(); // <1>
+        Settings persistentSettings = Settings.builder().put(persistentSettingKey, persistentSettingValue).build();
+        // end::update-settings-create-settings


I'm wondering if splitting this into 2 parts, one for the transient settings and one for the persistent ones would be more readable?

tlrx · 2018-02-13T11:08:15Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+            // tag::update-settings-async
+            client.cluster().updateSettingsAsync(request, listener); // <1>
+            // end::update-settings-async
+        }


Missing latch.await()

tlrx · 2018-02-13T11:09:25Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

+include-tagged::{doc-tests}/ClusterClientDocumentationIT.java[update-settings-request-cluster-settings]
+--------------------------------------------------
+<1> The transient settings provided as `Settigs`
+<2> The persistent settings provided as `Settigs`


Fxi Settigs ;)

tlrx · 2018-02-13T11:09:49Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

+--------------------------------------------------
+
+==== Cluster Settings
+At least one setting be updated must be provided:


"to be updated"?

tlrx · 2018-02-13T11:10:39Z

docs/java-rest/high-level/cluster/update_settings.asciidoc

+--------------------------------------------------
+include-tagged::{doc-tests}/ClusterClientDocumentationIT.java[update-settings-request-flat-settings]
+--------------------------------------------------
+<1> Returned settings in flat format


Could it be more descriptive?

olcbean · 2018-02-13T19:39:04Z

@javanna @tlrx thank you for the reviews! ( and sorry for the premature commit )

Now it should be ready for another round ;)

olcbean · 2018-02-13T19:42:13Z

docs/java-rest/high-level/cluster/put_settings.asciidoc

+--------------------------------------------------
+<1> Indicates whether all of the nodes have acknowledged the request
+<2> Indicates which transient settings have been applied
+<3> Indicates which persistent settings have been applied


I would like to shorten these ones by removing the leading Indicates. However it is used with Indicates in the indices section.

javanna · 2018-02-14T12:57:38Z

...level/src/test/java/org/elasticsearch/client/documentation/IndicesClientDocumentationIT.java

@@ -138,6 +138,8 @@ public void onFailure(Exception e) {
            // tag::indices-exists-async
            client.indices().existsAsync(request, listener); // <1>
            // end::indices-exists-async
+
+            assertTrue(latch.await(30L, TimeUnit.SECONDS));


javanna

thanks @olcbean I left some more comments.

javanna · 2018-02-14T12:58:41Z

.../main/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequest.java

+public class ClusterUpdateSettingsRequest extends AcknowledgedRequest<ClusterUpdateSettingsRequest> implements ToXContentObject {
+
+    private static final String PERSISTENT = "persistent";
+    private static final String TRANSIENT = "transient";


nit: we could make these constants of type ParseField and then below in toXContent call field.getPreferredName()

javanna · 2018-02-14T12:59:17Z

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexRequest.java

@@ -150,6 +154,8 @@ public GetIndexRequest includeDefaults(boolean includeDefaults) {

    /**
     * Whether to return all default settings for each of the indices.
+     * Used only by the high-level REST client.


thanks for these changes

javanna · 2018-02-14T12:59:35Z

...c/main/java/org/elasticsearch/rest/action/admin/cluster/RestClusterUpdateSettingsAction.java

-                        builder.endObject();
-                    }
-                });
+                new RestToXContentListener<ClusterUpdateSettingsResponse>(channel));


can you remove the generic type? <> should be enough

javanna · 2018-02-14T13:00:38Z

.../java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java

+        final ClusterUpdateSettingsRequest request = createTestItem();
+        boolean humanReadable = randomBoolean();
+        final XContentType xContentType = XContentType.JSON;
+        BytesReference xContent = toShuffledXContentAndInsertRandomFields(request, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);


inserting random fields is not a good idea for requests. we should parse them strictly. In fact if this test succeeds it means that we have a bug :) we should rather test that we are strict when parsing requests.

I added a test to make sure that inserting random fields fails here. Or is it too much?

javanna · 2018-02-14T13:02:52Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

+        BytesReference originalBytes = toShuffledXContent(toXContent, xContentType, params, humanReadable, exceptFieldNames);
+        BytesReference mutated;
+        if (randomBoolean()) {
+            mutated = insertRandomFields(xContentType, originalBytes, null, random());


given that in most cases we have two different test methods for this (with and without random fields), maybe we should not add this new method and rather do it like we did up until now? That said, I think I was not very diligent on this :)

javanna · 2018-02-14T13:12:21Z

...java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsResponseTests.java

+        persistentBuilder.keys().forEach(k -> {
+            assertThat(parsedPersistentBuilder.keys(), hasItem(equalTo(k)));
+            assertThat(parsedPersistentBuilder.get(k), equalTo(persistentBuilder.get(k)));
+        });


trying to figure out what happens when random fields are inserted among settings. I would expect the test to fail somehow, or maybe the arbitrary fields get parsed as setting and that is why you don't compare the two maps directly?

I wonder if randomizing at this level should be disabled, as its goal would be to verify that we are forward compatible when parsing, meaning that we will ignore a field/object if we don't know about it, as it could be a field added in a more recent version.

The Settings will be simply parsed ( no strict validation ) and during execution will be compared tp the supported settings. I will disable the randomization for the settings, then the check is only for the 'top-level' fields.

javanna · 2018-02-14T13:13:33Z

.../main/java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequest.java

+    private static final String TRANSIENT = "transient";
+
+    private static final ObjectParser<ClusterUpdateSettingsRequest, Void> PARSER = new ObjectParser<>("cluster_update_settings_request",
+            true, ClusterUpdateSettingsRequest::new);


ignoreUnknownFields should be set to false here, as it is a request

javanna · 2018-02-14T13:16:19Z

docs/java-rest/high-level/cluster/put_settings.asciidoc

@@ -0,0 +1,129 @@
+[[java-rest-high-cluster-put-settings]]
+=== Cluster Put Settings API


I am sorry, can you change the title to Cluster Update Settings API, like we have in the es docs? Confusing, I know :)

javanna · 2018-02-14T13:17:50Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+
+        String persistentSettingKey = EnableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ENABLE_SETTING.getKey();
+        String persistentSettingValue = EnableAllocationDecider.Allocation.NONE.name();
+        Settings persistentSettings = Settings.builder().put(persistentSettingKey, persistentSettingValue).build(); // <2>


would you mind wrapping these lines so they look better in the rendered docs?

wrapped for better doc display

javanna

I left a few nits, this is ready though.

javanna · 2018-02-15T09:58:34Z

...level/src/test/java/org/elasticsearch/client/documentation/ClusterClientDocumentationIT.java

+                .put(persistentSettingKey, persistentSettingValue)
+                .build(); // <2>
+        // end::put-settings-create-settings
+        // @formatter:on


these are eclipse specific tags right? I don't think we'd want them in our code.

According to the docs IntelliJ also supports them.
By default these tags are ignored ( at least for eclipse formatters ). I added them to make clear that these sections intentionally are not following the ES formatting rules.

Should I remove them?

yes please I haven't seen them used before in our codebase. At some point we will automate formatting and these classes will have to somehow be ignored I think.

javanna · 2018-02-15T10:02:04Z

.../java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java

+                    p -> p.startsWith("transient") || p.startsWith("persistent"), random());
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                    () -> ClusterUpdateSettingsRequest.fromXContent(createParser(xContentType.xContent(), mutated)));
+            assertTrue(e.getMessage().matches("\\[cluster_update_settings_request\\] unknown field \\[\\w*\\], parser not found"));


maybe instead of randomizing, you could just have added a specific test method with a non randomized json that contains un unsupported field, to verify that an error is thrown. Just to simplify things a bit.

javanna · 2018-02-15T10:02:51Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -146,6 +146,7 @@
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonList;
 import static org.elasticsearch.common.util.CollectionUtils.arrayAsArrayList;
+import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;


javanna · 2018-02-15T10:04:16Z

docs/java-rest/high-level/cluster/put_settings.asciidoc

+[[java-rest-high-cluster-put-settings]]
+=== Cluster Update Settings API
+
+The Cluster Put Settings API allows to update cluster wide settings.


s/Put/Update ?

javanna · 2018-02-15T10:05:03Z

jenkins please test this

correct docs remove tags disabling auto formatting

javanna · 2018-02-15T12:51:59Z

retest this please

javanna · 2018-02-15T16:21:56Z

thanks again @olcbean !

Relates to #27205

olcbean commented Feb 12, 2018

View reviewed changes

tlrx added the :Java High Level REST Client label Feb 13, 2018

javanna added the >enhancement label Feb 13, 2018

javanna requested changes Feb 13, 2018

View reviewed changes

javanna changed the title ~~[WIP] Add Cluster Update Settings API to the high level REST client~~ Add Cluster Update Settings API to the high level REST client Feb 13, 2018

javanna mentioned this pull request Feb 13, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

tlrx reviewed Feb 13, 2018

View reviewed changes

olcbean commented Feb 13, 2018

View reviewed changes

olcbean changed the title ~~Add Cluster Update Settings API to the high level REST client~~ Add Cluster Put Settings API to the high level REST client Feb 14, 2018

javanna reviewed Feb 14, 2018

View reviewed changes

javanna requested changes Feb 14, 2018

View reviewed changes

olcbean added 4 commits February 15, 2018 10:42

Add Cluster Update Settings API to the high level REST client

9378473

address reviewers' comments

a930033

address reviewer comments

432b659

Turn off the formatter for all sections which were

dda7951

wrapped for better doc display

olcbean force-pushed the hlrc_cluster_update_settings branch from 754e38a to dda7951 Compare February 15, 2018 09:43

javanna approved these changes Feb 15, 2018

View reviewed changes

modify tests

131a05f

correct docs remove tags disabling auto formatting

olcbean force-pushed the hlrc_cluster_update_settings branch from ef66147 to 131a05f Compare February 15, 2018 12:04

javanna added v7.0.0 v6.3.0 labels Feb 15, 2018

javanna merged commit 02fc16f into elastic:master Feb 15, 2018

javanna added the backport pending label Feb 15, 2018

javanna pushed a commit that referenced this pull request Feb 15, 2018

Add Cluster Put Settings API to the high level REST client (#28633)

5f5f430

Relates to #27205

javanna removed the backport pending label Feb 15, 2018

olcbean deleted the hlrc_cluster_update_settings branch February 16, 2018 12:14

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

		@@ -0,0 +1,129 @@
		[[java-rest-high-cluster-put-settings]]
		=== Cluster Put Settings API

Add Cluster Put Settings API to the high level REST client #28633

Add Cluster Put Settings API to the high level REST client #28633

Conversation

olcbean commented Feb 12, 2018 • edited Loading

elasticmachine commented Feb 12, 2018

elasticmachine commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a 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

olcbean Feb 13, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a 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

Choose a reason for hiding this comment

javanna left a 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

javanna commented Feb 15, 2018

javanna commented Feb 15, 2018

javanna commented Feb 15, 2018

olcbean commented Feb 12, 2018 •

edited

Loading

olcbean Feb 13, 2018 •

edited

Loading