Add Cluster Put Settings API to the high level REST client by olcbean · Pull Request #28633 · elastic/elasticsearch

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

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

s/Indices API/Cluster API

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

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

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

s/transiet/transient

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

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

s/transiet/transient

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

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

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

shall we move RestClusterUpdateSettingsAction to use RestToXContentListener?

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

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

rename the file to put_settings?

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

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

s/clustre/cluster

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

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

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

you mean as a map?

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

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

cluster api -> Cluster API

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

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

Fix Clustre

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

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

resetRequest

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

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

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

Missing latch.await()

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

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

Fxi Settigs ;)

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

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

"to be updated"?

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

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

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

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

javanna

thanks @olcbean I left some more comments.

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

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

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

thanks for these changes

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

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

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

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

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

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

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

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

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

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

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

 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 suggested 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

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

Copy link
Copy Markdown

Contributor

javanna Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ops :)

javanna suggested 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

Conversation

olcbean commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 12, 2018

Uh oh!

elasticmachine commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olcbean Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olcbean commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olcbean commented Feb 12, 2018 •

edited

Loading

olcbean Feb 13, 2018 •

edited

Loading