REST high-level client: add support for Indices Update Settings API #28892

olcbean · 2018-03-03T20:07:27Z

Related to #27205

elasticmachine · 2018-03-03T20:07:29Z

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-03-03T20:07:29Z

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?

javanna

left a couple of comments, LGTM though

javanna · 2018-03-05T13:45:12Z

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

@@ -0,0 +1,142 @@
+[[java-rest-high-indices-put-settings]]
+=== Update Indices Settings API


Shall we call the page "Update Settings API" given that it's under the Indices API group already?

👍 I would prefer to shorten it

But https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-update-settings.html
is using Update Indices Settings and currently the high level rest client docs are using the same names as the ones in the reference docs...

To keep the consistency between the client and the ref docs, maybe the ref docs should be changed as well?

@javanna what do you think?

leave it as-is then :)

javanna · 2018-03-05T13:45:38Z

rest-api-spec/src/main/resources/rest-api-spec/api/indices.put_settings.json

+        "timeout": {
+          "type" : "time",
+          "description" : "Explicit operation timeout"
+        },


good catch, thanks

Relates to #27158

javanna · 2018-03-05T13:49:54Z

...src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java


 /**
 * Request for an update index settings action
 */
-public class UpdateSettingsRequest extends AcknowledgedRequest<UpdateSettingsRequest> implements IndicesRequest.Replaceable {
+public class UpdateSettingsRequest extends AcknowledgedRequest<UpdateSettingsRequest>


should we also move its parsing code from the rest action and add a test UpdateSettingsRequestTests that tests both parsing and printing out from/to xcontent?

javanna · 2018-03-05T13:50:25Z

test this please

javanna · 2018-03-19T17:17:40Z

test this please

javanna

I left a few comments, mainly on testing. I will merge once those are addressed

javanna · 2018-03-20T09:55:00Z

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

+        parameters.withFlatSettings(updateSettingsRequest.flatSettings());
+        parameters.withPreserveExisting(updateSettingsRequest.isPreserveExisting());
+
+        String endpoint = buildEndpoint("_settings");


Along with the changes I made in #28866, this would become something like this:

String[] indices = updateSettingsRequest.indices() == null ? Strings.EMPTY_ARRAY : updateSettingsRequest.indices(); String endpoint = endpoint(indices, "_settings");

javanna · 2018-03-20T10:02:01Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestUpdateSettingsAction.java

-            }
-        }
-        updateSettingsRequest.settings(settings);
+        request.applyContentParser(parser -> updateSettingsRequest.fromXContent(parser));


this makes RequestsWithoutContentIT#testPutSettingsMissingBody fail, as applyContentParser doesn't require a response body. That is only because of a different error message though, as we now fail during request validation rather than on the REST layer. I think that failing earlier was better though. Can you do updateSettingsRequest.fromXContent(request.contentParser()); instead?

javanna · 2018-03-20T10:09:38Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+                    return builder;
+                }
+            };
+            bytesRef = toShuffledXContent(requestWithEnclosingSettings, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);


this could be part of createTestItem?

javanna · 2018-03-20T10:10:02Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+
+import static org.hamcrest.CoreMatchers.equalTo;
+
+public class UpdateSettingsRequestTests extends ESTestCase {


would it be possible to extend AbstractStreamableXContentTestCase ?

That would be better! But now the equals and the hachCode look "funny" ( only the serialized content is included ). Do you have an idea how to go about this?

I just committed another version where the equals and hashCode look correct and the test code for the serialization has been tweaked. I find the second approach cleaner and we should go with it. Do you agree ?

javanna · 2018-03-20T10:12:19Z

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

+[[java-rest-high-indices-put-settings-response]]
+==== Update Indices Settings Response
+
+The returned `UpdateSettings` allows to retrieve information about the


UpdateSettingsResponse ?

javanna · 2018-03-20T10:13:35Z

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

@@ -1227,6 +1228,34 @@ public void testRollover() throws IOException {
        assertEquals(expectedParams, request.getParameters());
    }

+    public void testIndexPutSettings() throws IOException {
+        String[] indices = randomIndicesNames(0, 2);
+        UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(indices);


Can we also test the case where indices are not set at all, then the request would hold null?

olcbean · 2018-03-20T10:53:08Z

@javanna thanks for the review!
Is it ok with you to rebase before I continue working on this one ?

javanna · 2018-03-20T11:22:20Z

@olcbean no worries, you can either merge master in or rebase, I don't mind.

…ettings

rewrite the PutSettingsRequestTests to extend from AbstractStreamableXContentTestCase

…ettings

olcbean · 2018-03-24T11:57:12Z

@javanna can you have another look ?

… tests

javanna · 2018-03-26T12:56:38Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+    @Override
+    protected void assertEqualInstances(UpdateSettingsRequest expectedInstance, UpdateSettingsRequest newInstance) {
+        newInstance.indices(expectedInstance.indices());
+        super.assertEqualInstances(expectedInstance, newInstance);


I see now better what I made you do! I am sorry, I think that I don't like it :) Our requests don't fit well with this way of testing. While responses are usually entirely serialized both on the transport layer and on the REST layer, with requests the toXContent is only the request body, but there's also the query_string params that are serialized at transport but not as XContent. This makes for impossible equals/hashcode and complicated testing. To do things right, here we should probably have two different tests, one for the Xcontent part and one for the serialization. I am wondering though if this is becoming besides the scope of this PR. I am totally fine with going back to testing only XContent here. Thanks for trying, and sorry for the back and forth.

No worries, it is fun to experiment :) Now we know that it is better to keep the *RequestTests extend ESTestCase ( or maybe AbstractStreamableTestCase but extending from AbstractStreamableXContentTestCase makes the tests tricky to maintain )

I still kept the serialization tests and went back to explicit to/from XContent testing.

olcbean · 2018-03-27T14:04:48Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+            BytesReference originalBytes = toShuffledXContent(requestWithEnclosingSettings, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
+            if (randomBoolean()) {
+                Predicate<String> excludeFilter = (s) -> s.startsWith("settings");
+                bytesRef = insertRandomFields(xContentType, originalBytes, excludeFilter, random());


I noticed something strange in the UpdateSettingsRequest#fromXContent ( maybe not as part of this PR but it is easy to show it here ;) )

If the request is something like :

{ "field1" : "value1", "field2" : "value2", "settings" : { "field3" : "value3" } }

only "field3" : "value3" is parsed as a setting in the request ( exposed here by the insertRandomFields : inserting a random object at the top level is silently ignored )

Do we want such leniency in the requests ?
Do we want to keep on supporting the possibility to wrap the settings in "settings" ?

great catch, I would assume that we want to get rid of this but it's a potentially breaking change. Would you mind opening a new issue for this please?

javanna · 2018-03-27T14:27:12Z

retest this please

javanna · 2018-03-27T14:35:41Z

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

+        // end::put-settings-request
+
+        // tag::put-settings-create-settings
+        String settingKey = IndexMetaData.SETTING_NUMBER_OF_REPLICAS;


nit: shall we just use the string here? IndexMetaData is quite an internal class that will go away once the high-level REST client won't depend on Elasticsearch anymore. I wouldn't want to have users copy paste this.

javanna · 2018-03-27T14:36:24Z

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

+--------------------------------------------------
+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[put-settings-request-flat-settings]
+--------------------------------------------------
+<1> Wether the updated settings returned in the `UpdateSettings` should


javanna · 2018-03-27T14:48:44Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+        super.assertEqualInstances(expectedInstance, newInstance);
+    }
+
+    public void testXContent() throws IOException {


do you think that it would be possible to have two different test classes, one for the xcontent bits, that doesn't rely on equals/hashcode , and the other one like this on that we have that tests serialization and ordinary equals/hashcode ? Let me know if I am missing something.

address reviewer remarks

olcbean · 2018-03-27T16:13:09Z

@javanna can you have another look ?

javanna · 2018-03-27T20:10:57Z

...rg/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestStreamableTests.java

+    @Override
+    protected void assertEqualInstances(UpdateSettingsRequest expectedInstance, UpdateSettingsRequest newInstance) {
+        newInstance.indices(expectedInstance.indices());
+        super.assertEqualInstances(expectedInstance, newInstance);


shouldn't we rely on the ordinary equals/hashcode here? But also preserveExisting and indicesOptions should be taken into account?

javanna · 2018-03-27T20:13:23Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
+import static org.hamcrest.Matchers.equalTo;
+
+public class UpdateSettingsRequestTests extends ESTestCase {


could we extend AbstractXContentTestCase ?

AFAICS the AbstractXContentTestCase will use the #assertEqualInstances and then I will need to "exclude" everything but the settings from the equals / hashCode ( similar to https://github.com/olcbean/elasticsearch/blob/32454d96e71a68f395bfe20320a0018f1d1ecc10/server/src/test/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java#L65-L68 ). Hence I opted for the basic ESTestCase.

What do you think ?

the nice thing of it is that you wouldn't need to write the usual code to test parsing etc. indeed in this case you would have to rewrite assertEqualInstances to not rely on equals/hashcode so that we only check stuff that is serialized over xcontent. I would give this a try if you don't mind, unless there are complications that I don't see :)

Testing the enclosing "settings" gets a little bit more complicated.

I made two commits with a simple version for the XContentTests and the last one includes the enclosing "settings" ( as I am not sure if we actually want to test for enclosing "settings" )

request

javanna · 2018-03-29T16:23:02Z

retest this please

javanna

left a couple of nits, LGTM otherwise, thanks for your patience once again ;)

javanna · 2018-03-29T18:55:59Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+
+    @Override
+    protected boolean supportsUnknownFields() {
+        return enclosedSettings;


I wonder if we should test this, in general requests should always be strict, unless they accept key-value pairs like in this case with settings. I think that I would always return false here

I am fine either way. I decided to leave it here till #29268 is fixed and the enclosing "settings" are removed, then the test can be really simplified :)

IMHO for any request this should be set to false : any object insertion at the top level should make the parsing fail ( for predefined field names ).

Off-topic : should we consider extending the AbstractXContentTestCase to explicitly test that if supportsUnknownFields is false actually an exception is thrown when an object is inserted at the top-level ? ( this can lead to a lot of changes in the tests and maybe the code, but could help determine how strict the rest request parsing is... )

I would set it to false too. I think that what you propose is a good idea, we have been using that flag only to test that we are lenient with responses, but we could use it also to test that we are strict.

what I had missed is that we cannot set it to false unless we get rid of the leniency caused by parsing the settings object etc.

Do you want to open an issue about this ?

yes makes sense to me.

javanna · 2018-03-29T18:56:07Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+
+public class UpdateSettingsRequestTests extends AbstractXContentTestCase<UpdateSettingsRequest> {
+
+    private boolean enclosedSettings = randomBoolean();


nit: make this final?

javanna · 2018-03-29T18:57:10Z

...est/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

+
+    @Override
+    protected void assertEqualInstances(UpdateSettingsRequest expectedInstance, UpdateSettingsRequest newInstance) {
+        super.assertEqualInstances(new UpdateSettingsRequest(expectedInstance.settings()),


can you leave a short comment here to explain why we do this? Smart way of doing it btw!

olcbean · 2018-03-30T08:47:43Z

@javanna thanks for the reviews !
Can you have another look ?

javanna · 2018-03-30T08:53:35Z

Thanks @olcbean !

…28892) Relates to #27205

…gs API (elastic#28892)" This reverts commit b67b5b1.

…gs API (#28892)" (#29323) This reverts commit b67b5b1.

mayya-sharipova · 2018-03-30T23:31:01Z

@olcbean @javanna I am sorry, I have to revert this PR from master and 6.x. We have tests of x-pack build failing because of this change.

Citing @jaymode here:

The issue here is an effect of #28892, which changes the equals/hashCode contract of MasterNodeRequest and in turn every other request that extends MasterNodeRequest. Previously this test relied upon the fact that there was an identity comparison so when running two ClusterHealthRequests and verifying the auditing there was only a single invocation; now that the two ClusterHealthRequests are treated as being "equal" to each other, even though they are not, the test fails because it thinks the same method was invoked twice with the same arguments.

I would suggest modify equality methods in MasterNodeRequest to address this.
Sorry again, and let me know how I can help.

…gs API (#28892)" This reverts commit e917e85.

olcbean · 2018-03-31T11:13:49Z

Ops!

@mayya-sharipova I just opened a PR removing the equals for the MasterNodeRequest ( #29327 ). This should fix the problem. Hopefully ;)

I do not know what tests are affected and / or the test infrastructure but if the tests in the x-pack expect the default ( identity ) equals, maybe it would be better to call == explicitly ?

mayya-sharipova · 2018-04-02T14:05:44Z

@olcbean thanks! We can ask @javanna to continue with the review of the new PR, when he has available time.

* master: (80 commits) Remove HTTP max content length leniency (elastic#29337) Begin moving XContent to a separate lib/artifact (elastic#29300) Java versions for ci (elastic#29320) Minor cleanup in the InternalEngine (elastic#29241) Clarify expectations of false positives/negatives (elastic#27964) Update docs on vertex ordering (elastic#27963) Revert "REST high-level client: add support for Indices Update Settings API (elastic#28892)" (elastic#29323) [test] remove Streamable serde assertions (elastic#29307) Improve query string docs (elastic#28882) fix query string example for boolean query (elastic#28881) Resolve unchecked cast warnings introduced with elastic#28892 REST high-level client: add support for Indices Update Settings API (elastic#28892) Search: Validate script query is run with a single script (elastic#29304) [DOCS] Added info on WGS-84. Closes issue elastic#3590 (elastic#29305) Increase timeout on Netty client latch for tests Build: Use branch specific refspec sysprop for bwc builds (elastic#29299) TEST: trim unsafe commits before opening engine Move trimming unsafe commits from engine ctor to store (elastic#29260) Fix incorrect geohash for lat 90, lon 180 (elastic#29256) Do not load global state when deleting a snapshot (elastic#29278) ...

REST high-level client: add support for Indices Update Settings API

97b0bdc

javanna self-assigned this Mar 5, 2018

javanna added :Core/Java High Level REST Client >enhancement labels Mar 5, 2018

javanna mentioned this pull request Mar 5, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna approved these changes Mar 5, 2018

View reviewed changes

UpdateSettingsRequest : add fromXContent and toXContent

a7c44ba

javanna requested changes Mar 20, 2018

View reviewed changes

javanna mentioned this pull request Mar 20, 2018

update index settings #29142

Closed

olcbean added 3 commits March 22, 2018 11:09

Merge remote-tracking branch 'origin/master' into hlrc_update_index_s…

874f576

…ettings

avoid possible NPE if there are no indices

e944714

rewrite the PutSettingsRequestTests to extend from AbstractStreamableXContentTestCase

Merge remote-tracking branch 'origin/master' into hlrc_update_index_s…

624423d

…ettings

olcbean added 2 commits March 25, 2018 11:41

do not compare the indices for the serialization and to/from XContent…

32454d9

… tests

remove an unnecessary checkstyle suppression

4f8109c

javanna reviewed Mar 26, 2018

View reviewed changes

going back to an explicit ToFromXContent test

74cbb30

olcbean commented Mar 27, 2018

View reviewed changes

javanna reviewed Mar 27, 2018

View reviewed changes

Split the UpdateSettingsRequestTests in two

2ac8ddb

address reviewer remarks

javanna reviewed Mar 27, 2018

View reviewed changes

olcbean added 3 commits March 28, 2018 12:25

extend the equals and cacheCode to cover all object fields

a4982a8

complete streamable test; simple XContenet test

c7aa404

modify the XContent tests to randomly test for enclosing settings in the

f95231f

request

javanna approved these changes Mar 29, 2018

View reviewed changes

adding few more comments

f9d8914

javanna merged commit b67b5b1 into elastic:master Mar 30, 2018

javanna added v7.0.0 v6.3.0 labels Mar 30, 2018

javanna added a commit that referenced this pull request Mar 30, 2018

Resolve unchecked cast warnings introduced with #28892

bcc9cbf

javanna added the backport pending label Mar 30, 2018

javanna pushed a commit that referenced this pull request Mar 30, 2018

REST high-level client: add support for Indices Update Settings API (#…

e917e85

…28892) Relates to #27205

javanna added a commit that referenced this pull request Mar 30, 2018

Resolve unchecked cast warnings introduced with #28892

5b9ed2d

javanna removed the backport pending label Mar 30, 2018

mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Mar 30, 2018

Revert "REST high-level client: add support for Indices Update Settin…

90a45ae

…gs API (elastic#28892)" This reverts commit b67b5b1.

mayya-sharipova added a commit that referenced this pull request Mar 30, 2018

Revert "REST high-level client: add support for Indices Update Settin…

e70cd35

…gs API (#28892)" (#29323) This reverts commit b67b5b1.

mayya-sharipova added a commit that referenced this pull request Mar 30, 2018

Revert "REST high-level client: add support for Indices Update Settin…

e5aefe4

…gs API (#28892)" This reverts commit e917e85.

olcbean mentioned this pull request May 18, 2018

Add Get Aliases API to the high-level REST client #28799

Merged

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

		@@ -0,0 +1,142 @@
		[[java-rest-high-indices-put-settings]]
		=== Update Indices Settings API


		import static org.hamcrest.CoreMatchers.equalTo;

		public class UpdateSettingsRequestTests extends ESTestCase {


		public class UpdateSettingsRequestTests extends AbstractXContentTestCase<UpdateSettingsRequest> {

		private boolean enclosedSettings = randomBoolean();

REST high-level client: add support for Indices Update Settings API #28892

REST high-level client: add support for Indices Update Settings API #28892

Conversation

olcbean commented Mar 3, 2018

elasticmachine commented Mar 3, 2018

elasticmachine commented Mar 3, 2018

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 Mar 5, 2018

javanna commented Mar 19, 2018

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

olcbean commented Mar 20, 2018

javanna commented Mar 20, 2018

olcbean commented Mar 24, 2018

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 Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Mar 27, 2018

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 Mar 29, 2018

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

olcbean commented Mar 30, 2018

javanna commented Mar 30, 2018

mayya-sharipova commented Mar 30, 2018

olcbean commented Mar 31, 2018

mayya-sharipova commented Apr 2, 2018