Add Get Settings API support to java high-level rest client #29229

tomcallahan · 2018-03-23T20:21:28Z

First-timer here so I am likely missing some things and could use some help understanding a few things.

I have HELP embedded in a comment in RequestTests.java where there is something going on that I definitely don't understand.
The *ResponseTests.java classes elsewhere that I looked at seem highly variable in how much they test. For instance, BulkResponseTests seems to just test the XContent logic (manually), CloseIndexResponseTests seems to use some precanned stuff for testing XContent, whereas ClusterHealthResponsesTests doesnt seem to have anything to do with serialization.
It looks like we are re-using the java objects that are being used currently for the transport client for the java rest client as well? I assume that this is part of the complexity of rest client dependencies?

Thanks for taking the time to take a look.

elasticmachine · 2018-03-23T20:22:46Z

Pinging @elastic/es-core-infra

javanna

I did a first pass and left some comments, thanks @tomcallahan !

javanna · 2018-03-23T21:06:13Z

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

+
+        if (getSettingsRequest.indices().length == 0) {
+            throw new IllegalArgumentException("getSettings requires at least one index");
+        }


I think this validation is not needed: we also register the /_settings/{name} that doesn't require any index. We need to make the client go to that one endpoint if no indices are provided.

javanna · 2018-03-23T21:07:58Z

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

+    static Request getSettings(GetSettingsRequest getSettingsRequest) throws IOException {
+        Params params = Params.builder();
+        params.withIndicesOptions(getSettingsRequest.indicesOptions());
+        params.withLocal(getSettingsRequest.local());


there are a few other params that we need to provide. Unfortunately it's all params that the transport client doesn't support but only the ES REST layer knows about. What we've been doing in these situations is add those params to the request object, and document that only the high-level REST client uses them. We've already encountered flat_settings for instance , and this one also has include_defaults.

I definitely understand adding include_defaults -- but is there a reason to include flat_settings -- won't the behavior of the java API be identical whether that flag is set or not ?

You are right! I thought that somehow we would parse settings differently with or without the flag, but I have just tested it and nothing changes. So, ideally we would support this flag as it is part of our SPEC, but practically it does not do anything given how we parse settings back given that we use the Settings class. It would only make sense to expose this flag if we changed the way settings were returned. Thanks for pointing this out. I will also have the remove support for this flag from a couple of other API where we added it :(

javanna · 2018-03-23T21:09:08Z

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

+    public void testGetSettings() throws IOException {
+        String indexName = "get_settings_index";
+        Settings basicSettings = Settings.builder()
+            .put("number_of_shards", 13)


nit: could we create it with a bit less shards? I suspect it may make the test faster.

javanna · 2018-03-23T21:10:09Z

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

+        assertEquals(RestStatus.NOT_FOUND, exception.status());
+    }
+
+    public void testGetMultipleSettings() throws IOException {


you meant multiple indices maybe?

javanna · 2018-03-23T21:10:39Z

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

+    public void testGetSettingsFiltered() throws IOException {
+        String indexName = "get_settings_index";
+        Settings basicSettings = Settings.builder()
+            .put("number_of_shards", 13)


here too, if possible I would decrease the number of shards

javanna · 2018-03-23T21:21:07Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();


we should rather take what we already have in RestGetSettingsAction to print out the response and move it here to the response, then also call it from there. You will probably be able to also move from RestBuilderListener to RestToXContentListener. I think that we are currently losing the include_defaults part...but looking deeper at it , I am not sure how that part will work out in the response given its dependencies. Leave a TODO for now I will look deeper next week.

I had a deeper look at the settings filtering logic that we currently have in the REST layer, namely this part:

if (renderDefaults) { builder.startObject("defaults"); settingsFilter.filter(indexScopedSettings.diff(cursor.value, settings)).toXContent(builder, request); builder.endObject(); }

On one hand it is not a bad idea to keep the client code simple, you don't need filtering there as you just parse and print out what you get back from the server. On the other hand though I think we should have the rendering code in one place, rather than duplicated in the REST layer and in the response.

I am thinking that the filtering could be moved entirely to the transport layer here, based on the new includeDefaults flag that you are going to add to the request. In that case it will actually be read also from the transport client, and the response after such change will only contain the settings to be returned (and printed out) without needing an additional filtering pass. Let me know what you think about this.

javanna · 2018-03-23T21:30:31Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

@@ -72,4 +84,65 @@ public void writeTo(StreamOutput out) throws IOException {
            Settings.writeSettingsToStream(cursor.value, out);
        }
    }
+
+    public static GetSettingsResponse fromXContent(XContentParser parser) throws IOException {
+        HashMap<String, Settings> indexToSettings = new HashMap<>();


nit: declare a Map on the left side?

javanna · 2018-03-23T21:32:23Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+            indexToSettings.put(indexName, settings);
+        }
+        return new GetSettingsResponse(ImmutableOpenMap.<String, Settings>builder().putAll(indexToSettings).build());
+    }


nit: add an empty line between the two methods?

javanna · 2018-03-23T21:32:35Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+
+    @Override
+    public int hashCode() {
+


nit: remove this empty line?

javanna · 2018-03-23T21:35:34Z

.../test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java

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


would it be possible to rather leave the true default but override getRandomFieldsExcludeFilter so that fields are not inserted at the root, but as the same level as settings they are, and also not within settings? That is because indices and settings names are arbitrary, it doesn't really make sense to insert anything random in there. But at the level of settings, it may make sense. We usually do this to test forward compatibility, to make sure that whatever we may add to the response in the future doesn't break things. In fact I think that your parsing code may be strict to that regard.

I attempted to do so -- however it appears that the random fields that get embedded may be fields that the parser is totally unprepared to deal with - for instance, embedding a value object where one is not expected.

I suspect that if the test fails it's because your parsing code needs to be adjusted to ignore what it does not know about. We do this in response parsing to ensure forward compatibility.

javanna · 2018-03-23T21:43:28Z

The *ResponseTests.java classes elsewhere that I looked at seem highly variable in how much they test. For instance, BulkResponseTests seems to just test the XContent logic (manually), CloseIndexResponseTests seems to use some precanned stuff for testing XContent, whereas ClusterHealthResponsesTests doesnt seem to have anything to do with serialization.

True, some of these inconsistencies may be fixed, but for instance BulkResponseTests needs to be quite different and cannot inherit from a different base class. Things become very tricky as soon as a response holds exceptions, as they cannot be parsed exactly into what they were originally, and e.g. AbstractStreamableXContentTestCase requires equals/hashcode which cannot be easily implemented with such responses. Serialization unit tests may be missing for quite some requests and responses, whenever we add support for a new API to the high-level RESTA client we try to introduce them by using AbstractStreamableXContentTestCase like you did, but it's not always possible for the reason I mentioned and we don't want to divert too much from the original goal of this effort.

It looks like we are re-using the java objects that are being used currently for the transport client for the java rest client as well? I assume that this is part of the complexity of rest client dependencies?

Yes. In short, we went for reusing which meant carrying some legacy with us. But we are closer to what the transport client did before so it should be easier to migrate for users.

javanna · 2018-03-26T11:00:58Z

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

+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[get-settings-request-masterTimeout]
+--------------------------------------------------
+<1> Timeout to connect to the master node as a `TimeValue`
+<2> Timeout to connect to the master node as a `String`


remove master_timeout here and add the missing arguments

javanna · 2018-03-26T11:18:22Z

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

+        // tag::get-settings-response
+        String numberOfShardsString = getSettingsResponse.getSetting("index", "index.number_of_shards"); // <1>
+        Settings indexSettings = getSettingsResponse.getIndexToSettings().get("index"); // <2>
+        int numberOfShards = indexSettings.getAsInt("index.number_of_shards", null); // <3>


this is not a great example I'm afraid, as you set a default value to null, but given that the variable is int a null pointer will be thrown

javanna · 2018-03-26T11:30:22Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+            builder.startObject();
+            settings.toXContent(builder, params);
+            builder.endObject();
+            builder.endObject();


for readability purposes, would you mind doing something like the following?

builder.startObject(indexName); { builder.startObject("settings"); settings.toXContent(builder, params); builder.endObject(); } builder.endObject();

javanna · 2018-04-03T11:02:13Z

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

+        String[] indicesUnderTest = java.util.stream.Stream.concat(
+            java.util.Arrays.stream(randomIndices),
+            java.util.stream.Stream.of(emptyIndexName, nullIndexName)
+        ).toArray(String[]::new);


can you rather do this like we do in other similar methods, along the lines as the following snippet?

String[] indices = randomBoolean() ? null : randomIndicesNames(0, 5); FlushRequest flushRequest; if (randomBoolean()) { flushRequest = new FlushRequest(indices); } else { flushRequest = new FlushRequest(); flushRequest.indices(indices); }

This one confuses me - you asked me to ensure that I tested null and empty index names, your snippet does not appear to do that. Further, the snippet itself smells -- the if-conditional appears to simply take one of two routes to constructing an object that should be the same either way. If we want to test our constructor logic, a unit test would accomplish that more simply

Don't we test null by randomly setting indices to null, and empty by using 0 as a lower bound when calling randomIndicesNames(0, 5)? Maybe we mean to test two different things here.

I asked this because we previously had bugs in case indices were set to null, as in NPE when converting the request object.

The if is probably not so important but it just makes sure that however you set the indices (there are two ways to do it), the behaviour is the same. We are not really testing the request here or its constructor, but rather that we can convert the request object (e.g. FlushRequest) into our own proper internal Request representation. I suppose we could take the if out if we wanted to, as we effectively rely on the indices getter here. I don't think testing the two codepaths hurts though either.

Could you elaborate on what smells? I don't follow.

Yes - I misinterpreted what you said - i thought you wanted me to test when the index name itself was null, or when the index name was "" (empty string). I can absolutely test for when the list of index names are null or empty.

The smell for me is in doing a /random/ test of the two ways we can set indices -- it seems to me that objective is better accomplished using a unit test that just runs all of the time and asserting that the request objects are identical.

you are right on this, randomized tests need to be used with care, and sometimes we use them to have better coverage with a reduced amount of tests. Let's say this if addresses some paranoia of mine given that there are two ways to set indices, but I am too lazy to write two different tests for this, especially given that we are not testing the request object in this test :) It's an interesting (and potentially long) discussion, I don't mind if you want to get rid of the if.

Should we really be test for null and empty here? We don't do that in all of the RequestTests -- the way the rest endpoint is structured null and empty should not be possible, and we're going to ugly a few things up if we try to account for that

This request is a bit different from the previous BroadcastRequests that we already test this for. The reason is that the serialization for GetSettingsRequest code assumes a non null value is set, and the default value is non null. We could then simply not accept null when it gets set (failing early is better). As for empty, I think it's a perfectly valid value and we should test it?

But we do test this in RequestTests for other requests.

The reason why I tend to test these corner cases is that we had issues before, as we were trying to concatenate null indices with the rest of the endpoint when building the final url. We need to make sure that we output the proper endpoint and we don't break with whatever value the request allows to set. I don't follow what the REST endpoint has to do with this, we are using the request from the client, outside of Elasticsearch.

javanna · 2018-04-03T11:11:08Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+            return Strings.toString(builder);
+        } catch (IOException e) {
+            throw new IllegalStateException(e); //should not be possible here
+        }


why not using Strings#toString(ToXContent) here?

I see why but I am not sure that the new flag that you pass to toXContent helps that much, given that it's only used in the toString method? I think that we can do without it and keep things simple. what do you think?

I'd recommend keeping it -- the reason is that toString() method is called and displayed for both the expected and actual objects during tests. If we "lie" in toString(), the output in the case of a test failure may appear to be identical when in fact it is not. I ran into this during testing and it is precisely why I made it this way (I had it as a plain toXContent before).

I didn't realize that the toString could lie without this added flag. I don't understand though completely why it's useful. Could you post an example of a situation where it helps please? Like what you have seen while testing.

Certainly - If you look at GetSettingsResponseTests.java, you'll see that on line 86 I manually add a refresh interval setting in all cases. If I left that out, RandomCreateIndexGenerator.randomIndexSettings() will occasionally return an empty settings object. So, we could have an indexToSettings map that looks like this:
{"WFrlf":{"settings":{"index":{"number_of_replicas":"9"}}},"QDjdX":{"settings":{}}}
However, toXContent will "optimize" away the the index with no settings. This will cause the equality test to fail, however as the toString leverages toXContent, the test failure will look like this:

Expected :{"WFrlf":{"settings":{"index":{"number_of_replicas”:”9”}}}} Actual :{"WFrlf":{"settings":{"index":{"number_of_replicas”:”9”}}}}

With this modification to toString(), it ensures that the test failure will be more grokable:

Expected :{"WFrlf":{"settings":{"index":{"number_of_replicas":"9"}}},"QDjdX":{"settings”:{}}} Actual :{"WFrlf":{"settings":{"index":{"number_of_replicas”:”9”}}}}

gotcha, thanks for the explanation! I'd prefer to simplify things a bit here. After all this is a corner case that happens only in our unit tests, an index will always have at least a couple of settings like number_of_shards, creation_date etc. I would rather move the if to the transport action, or even better, remove this optimization given that it buys us nothing (provided no tests fail after such change :) )

By the way, having no settings to show could happen in practice when one provides a settings_filter that doesn't match any settings. I think it would be more correct to still return the index in that case, with an empty set of settings. Problem is that at the moment the API doesn't accept settings_filter, and this is a bug that we should fix separately. Working on these little client PRs tends to uncover a lot of stuff :)

ok on settings_filter param that is not supported here, I will open a followup PR, I think that such param should not have been exposed anywhere after all. But we do allow to filter through the names parameter instead, so it is possible that we have no settings to return for some index. Let's keep this behaviour for backwards compatibility reasons but let's change it as a follow-up in master and get rid of this toXContent variant and toString impl?

that makes sense to me

javanna · 2018-04-03T11:15:51Z

...ain/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java

        for (Index concreteIndex : concreteIndices) {
            IndexMetaData indexMetaData = state.getMetaData().index(concreteIndex);
            if (indexMetaData == null) {
                continue;
            }

-            Settings settings = settingsFilter.filter(indexMetaData.getSettings());
+            Settings indexSettings = settingsFilter.filter(indexMetaData.getSettings());


nit: I am fine this rename, but doing it as part of this PR makes reviewing a bit trickier, it would be easier to spot what changed without it.

I did it as part of this PR as we also now need the this.settings object in order to perform the diff -- using settings and this.settings i feel really muddies the waters. I'll add a revision as you requested though and let you take a look.

it's ok, keep it. I have already looked at the changes and they are fine.

javanna · 2018-04-03T11:19:56Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+            parser::getTokenLocation);
+        if (settingsFieldName.equals("settings") == false && settingsFieldName.equals("defaults") == false) {
+            String message = "Failed to parse object: expecting field with name [%s] but found [%s]";
+            throw new org.elasticsearch.common.ParsingException(parser.getTokenLocation(), String.format(java.util.Locale.ROOT, message,


this makes parsing strict, which is good anywhere we do parsing in Elasticsearch besides response parsing :) that's because we would like the high-level REST client to work also against future versions of Elasticsearch, which may be adding new fields/arrays/objects to their responses.

makes good sense

javanna · 2018-04-03T11:23:42Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+        }
+        ImmutableOpenMap.Builder<String, Settings> defaultSettingsBuilder = ImmutableOpenMap.builder();
+
+        if (in.getVersion().onOrAfter(org.elasticsearch.Version.V_7_0_0_alpha1)) {


given that we are backporting this to 6.x I think this version should be changed to 6.3 (provided that it makes it before we cut that release). You can also add a TODO and do this later, let's just make sure we don't forget :)

The recommended strategy we use for BWC changes is to:

set the version to 7.0.0, get a green build including BWC

backport the change to 6.x and change the version to 6.3.0, get a green build including BWC

open a small change to master changing the version to 6.3.0, get a green building including BWC

If the version is changed to 6.3.0 now, it will not be possible to get a green build including BWC other than via silencing tests.

javanna · 2018-04-03T11:24:57Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

@@ -45,8 +66,17 @@ public GetSettingsResponse(ImmutableOpenMap<String, Settings> indexToSettings) {

    public String getSetting(String index, String setting) {
        Settings settings = indexToSettings.get(index);
+        Settings defaultSettings = indexToDefaultSettings.get(index);


maybe you can postpone this and do it only once you need it?

javanna · 2018-04-03T11:26:08Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

@@ -45,8 +66,17 @@ public GetSettingsResponse(ImmutableOpenMap<String, Settings> indexToSettings) {

    public String getSetting(String index, String setting) {


would you mind adding some javadoc and explaining that we look first among the settings, then among the default ones which are there only when includeDefaults was true in the request?

javanna · 2018-04-03T11:29:37Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java


-    public GetSettingsResponse(ImmutableOpenMap<String, Settings> indexToSettings) {
+    private static final ConstructingObjectParser<GetSettingsResponse, Void> PARSER = new ConstructingObjectParser<>(


I think that this is not used anywhere

javanna · 2018-04-03T11:30:45Z

...ain/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java

+
+            indexToSettingsBuilder.put(concreteIndex.getName(), indexSettings);
+            if (request.includeDefaults()) {
+                indexToDefaultSettingsBuilder.put(concreteIndex.getName(), indexScopedSettings.diff(indexSettings, this.settings));


don't we also need to call settingsFilter.filter on the result of the diff?

Perhaps! I assumed that the this.settings object would not contain anything that needed to be filtered, but I might be incorrect -- do you have any insights on how settingsFilter and this.settings are created?

I am not sure either, I would need to test this, I only saw that we were using it before in the REST action, but I wonder too if it was needed there.

javanna · 2018-04-03T11:32:13Z

...ain/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsAction.java

        }
-        listener.onResponse(new GetSettingsResponse(indexToSettingsBuilder.build()));
+        listener.onResponse(new GetSettingsResponse(indexToSettingsBuilder.build(), indexToDefaultSettingsBuilder.build()));


The default settings part could only be tested via REST tests or REST action unit tests before. Now that the logic is all in the transport action, it may be possible to write a unit test for this. It would be nice to have, also as a follow-up if this PR becomes too big with that.

I have included a unit test in the most recent PR update -- it definitely caught some bugs!

javanna · 2018-04-03T11:33:36Z

.../test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java

+        Assert.assertEquals(TEST_6_2_2_RESPONSE_BYTES, base64OfResponse);
+    }
+
+    public void testTransportSerdeRoundTripCurrentVersion() throws IOException {


TransportSerde? what does the suffix mean?

Serialization/Deserialization

javanna

I left some comments but this is definitely going in the right direction! thanks @tomcallahan !

javanna

heya @tomcallahan I did another round, this is getting closer and closer. Most of my comments are minor. Main bit is to make parsing of the response less strict and test forward compatibility on it. Thanks for all the rounds up until now, we definitely found some unknown unknowns on our way here :)

javanna · 2018-04-16T13:47:33Z

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

+        setRandomMasterTimeout(getSettingsRequest, expectedParams);
+        setRandomIndicesOptions(getSettingsRequest::indicesOptions, getSettingsRequest::indicesOptions, expectedParams);
+
+        setRandomLocal(getSettingsRequest, expectedParams);


can we also test includeDefaults and names here?

javanna · 2018-04-16T13:49:47Z

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

@@ -568,7 +592,7 @@ public void testIndex() throws IOException {
    }

    public void testRefresh() {
-        String[] indices = randomBoolean() ? null : randomIndicesNames(0, 5);
+        String[] indices = randomIndicesNames(1, 5);


maybe I told you to make this change, I forgot, but I wonder why. all BroadcastRequests can hold null or empty indices right?

This isn't even a method I should be changing -- I think as I was hacking I hacked in the wrong spot.

javanna · 2018-04-16T13:55:59Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+     *
+     * Returns the string value for the specified index and setting.  If the includeDefaults
+     * flag was not set or set to false on the GetSettingsRequest, this method will only
+     * return a value where the setting was explicitly set on the index.   If the includeDefaults


Remove a few too many whitespaces after the end of the sentence?

javanna · 2018-04-16T14:09:24Z

...r/src/main/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponse.java

+            builder.startObject("settings");
+            cursor.value.toXContent(builder, params);
+            builder.endObject();
+            if (!indexToDefaultSettings.isEmpty()) {


nit: we try to use == false instead of ! for checks like these, as the former can easily be missed. Would you mind changing this please?

makes sense, thanks

javanna · 2018-04-16T17:29:41Z

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

+        assertNull(getSettingsResponse.getSetting(indexName, "index.number_of_shards"));
+        assertEquals(0, getSettingsResponse.getIndexToSettings().get("get_settings_index").size());
+        assertEquals(1, getSettingsResponse.getIndexToDefaultSettings().get("get_settings_index").size());
+    }


thanks for adding all these different tests, these are great to have!

javanna · 2018-04-16T18:39:01Z

.../test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java

+    }
+
+    protected boolean supportsUnknownFields() {
+        return false;


this should be set to true, and we should exclude the index level objects from it. I thought it was that way before?

javanna · 2018-04-16T18:41:40Z

.../test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java

+
+public class GetSettingsResponseTests extends AbstractStreamableXContentTestCase<GetSettingsResponse> {
+
+    private final Set<String> indexNames;


this is only accessed in createTestItem right? If so why do we need it as an instance variable? Could we move the below block to createTestItem?

Good catch - one iteration did require it there but no longer

javanna · 2018-04-16T18:44:04Z

.../test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java

+        newResponse.readFrom(si);
+
+        Assert.assertEquals(responseWithExtraFields, newResponse);
+    }


Doesn't this test come for free if you extend AbstractStreamableXContentTestCase which you are doing? (testSerialization)

I don't believe so - I am testing the transport protocol in this case, not XContent

this base test class does both. testSerialization tests the transport serialization.

Got it, removed

javanna · 2018-04-16T18:44:58Z

.../test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsResponseTests.java

+        Assert.assertEquals(responseWithExtraFields, newResponse);
+    }
+
+    public void testRoundtrip622DropsDefaults() throws IOException {


doesn't this include testCanOutput622Response ?

you're correct, this test is technically superfluous

javanna · 2018-04-16T18:47:22Z

...t/java/org/elasticsearch/action/admin/indices/settings/get/TransportGetSettingsActionIT.java

+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.test.ESIntegTestCase;
+
+public class TransportGetSettingsActionIT extends ESIntegTestCase {


this is ok to have, but I meant a unit test while this one is more of an integration test as it requires to send requests to an Elasticsearch cluster. I would call it GetSettingsActionIT instead. Do you think we could have the same as a unit test, maybe as a followup?

I would be happy to but I can't tell how to get an instance of TransportGetSettingsAction -- any pointers on example unit tests?

If you look at, for example, TransportBulkActionTests, that will give you an idea how to construct a transport action for tests.

Thanks @jasontedor . That is helpful but does not quite get me all of the way there. I am concerned with this.settings and this.settingsFilter in particular in the action modules -- empty and/or dummies will not do, due to this line in TransportGetSettingsAction:

Settings defaultSettings = settingsFilter.filter(indexScopedSettings.diff(indexSettings, this.settings));

I could really use some help understanding where this.settings comes from and the purpose of it and this.settingsFilter. As far as I can tell, this.settings ultimately comes from the plugins service and is instantiated inside of Node.java. However, I am having trouble figuring out how index-scoped settings show up in there...

this.settings is node-level settings from startup; this will never have index-scoped settings in it.

I think you could pass in Settings.EMPTY when constructing this for the purpose of tests too and it will be fine.

Thanks @jasontedor that is making this make a bunch more sense

javanna · 2018-04-23T15:32:06Z

...c/test/java/org/elasticsearch/action/admin/indices/settings/get/GetSettingsRequestTests.java

+        byte[] responseBytes = BytesReference.toBytes(bso.bytes());
+        assertEquals(TEST_622_REQUEST_BYTES, Base64.getEncoder().encodeToString(responseBytes));
+    }
+    public void testDeserializeBackwardsCompatibility() throws IOException {


and add empty lines between the different methods?

javanna

heya @tomcallahan I did a last round of review, this loos ready, I left a bunch of formatting comments, there is a small issue in the docs, and a couple of previous comments that were not addressed. LGTM otherwise, feel free to merge once you have addressed those.

…-settings

tomcallahan · 2018-04-30T20:59:16Z

@elasticmachine test this please

…-settings

javanna · 2018-05-04T15:31:50Z

go @tomcallahan go!

Recently merged #29229 had a doc bug that broke the doc build. This commit fixes.

* master: Docs: remove transport_client from CCS role example (elastic#30263) [Rollup] Validate timezone in range queries (elastic#30338) Use readFully() to read bytes from CipherInputStream (elastic#28515) Fix docs Recently merged elastic#29229 had a doc bug that broke the doc build. This commit fixes. Test: remove cluster permission from CCS user (elastic#30262) Add Get Settings API support to java high-level rest client (elastic#29229) Watcher: Remove unneeded index deletion in tests

* master: (35 commits) DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1 Respect accept header on no handler (#30383) [Test] Add analysis-nori plugin to the vagrant tests [Docs] Fix bad link [Docs] Fix end of section in the korean plugin docs Expose the Lucene Korean analyzer module in a plugin (#30397) Docs: remove transport_client from CCS role example (#30263) [Rollup] Validate timezone in range queries (#30338) Use readFully() to read bytes from CipherInputStream (#28515) Fix docs Recently merged #29229 had a doc bug that broke the doc build. This commit fixes. Test: remove cluster permission from CCS user (#30262) Add Get Settings API support to java high-level rest client (#29229) Watcher: Remove unneeded index deletion in tests Set the new lucene version for 6.4.0 [ML][TEST] Clean up jobs in ModelPlotIT ...

That PR changed the execution path of index settings default to be on the master until the PR is back-ported the old master will not return default settings.

…r-you * origin/master: Update forcemerge.asciidoc (elastic#30113) Added zentity to the list of API extension plugins (elastic#29143) Fix the search request default operation behavior doc (elastic#29302) (elastic#29405) Watcher: Mark watcher as started only after loading watches (elastic#30403) Pass the task to broadcast actions (elastic#29672) Disable REST default settings testing until elastic#29229 is back-ported Correct wording in log message (elastic#30336) Do not fail snapshot when deleting a missing snapshotted file (elastic#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (elastic#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1

* origin/master: (39 commits) Docs: fix changelog merge Fix line length violation in cache tests Add stricter geohash parsing (elastic#30376) Add failing test for core cache deadlock [DOCS] convert forcemerge snippet Update forcemerge.asciidoc (elastic#30113) Added zentity to the list of API extension plugins (elastic#29143) Fix the search request default operation behavior doc (elastic#29302) (elastic#29405) Watcher: Mark watcher as started only after loading watches (elastic#30403) Pass the task to broadcast actions (elastic#29672) Disable REST default settings testing until elastic#29229 is back-ported Correct wording in log message (elastic#30336) Do not fail snapshot when deleting a missing snapshotted file (elastic#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (elastic#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1 ...

* elastic-master: Watcher: Mark watcher as started only after loading watches (#30403) Pass the task to broadcast actions (#29672) Disable REST default settings testing until #29229 is back-ported Correct wording in log message (#30336) Do not fail snapshot when deleting a missing snapshotted file (#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1 Respect accept header on no handler (#30383) [Test] Add analysis-nori plugin to the vagrant tests [Docs] Fix bad link [Docs] Fix end of section in the korean plugin docs Expose the Lucene Korean analyzer module in a plugin (#30397) Docs: remove transport_client from CCS role example (#30263) [Rollup] Validate timezone in range queries (#30338) Use readFully() to read bytes from CipherInputStream (#28515) Fix docs Recently merged #29229 had a doc bug that broke the doc build. This commit fixes. Test: remove cluster permission from CCS user (#30262) Add Get Settings API support to java high-level rest client (#29229) Watcher: Remove unneeded index deletion in tests

That PR changed the execution path of index settings default to be on the master until the PR is back-ported the old master will not return default settings.

Get Settings API changes have now been backported to version 6.4, and therefore the latest version must send and expect the extra fields when communicating with 6.4+ code. Relates elastic#29229 elastic#30494

….0 (#30706) Get Settings API changes have now been backported to version 6.4, and therefore the latest version must send and expect the extra fields when communicating with 6.4+ code. Relates #29229 #30494

….0 (elastic#30706) Get Settings API changes have now been backported to version 6.4, and therefore the latest version must send and expect the extra fields when communicating with 6.4+ code. Relates elastic#29229 elastic#30494

tomcallahan self-assigned this Mar 23, 2018

tomcallahan requested review from javanna and jasontedor March 23, 2018 20:21

javanna mentioned this pull request Mar 23, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

tomcallahan added >enhancement :Core/Java High Level REST Client labels Mar 23, 2018

javanna requested changes Mar 23, 2018

View reviewed changes

javanna reviewed Mar 26, 2018

View reviewed changes

tomcallahan requested review from nik9000 and removed request for jasontedor April 2, 2018 16:17

javanna reviewed Apr 3, 2018

View reviewed changes

javanna requested changes Apr 3, 2018

View reviewed changes

javanna changed the title ~~[WIP] Add Get Settings API support to java high-level rest client~~ Add Get Settings API support to java high-level rest client Apr 3, 2018

tomcallahan force-pushed the high-level-rest-get-settings branch from c4e02fe to 781f825 Compare April 11, 2018 18:17

javanna mentioned this pull request Apr 16, 2018

Settings filter not applied to default settings #29539

Closed

javanna requested changes Apr 16, 2018

View reviewed changes

javanna reviewed Apr 23, 2018

View reviewed changes

javanna approved these changes Apr 23, 2018

View reviewed changes

tomcallahan added 2 commits April 30, 2018 10:54

Incorporate reviewer feedback

e2a592e

Merge remote-tracking branch 'origin/master' into high-level-rest-get…

ccc6a72

…-settings

tomcallahan added 2 commits May 2, 2018 08:55

Merge remote-tracking branch 'origin/master' into high-level-rest-get…

4611fae

…-settings

fix item missed in merge

cfe8f2e

jasontedor approved these changes May 3, 2018

View reviewed changes

tomcallahan merged commit 0a93956 into elastic:master May 4, 2018

tomcallahan added v7.0.0 backport pending v6.4.0 labels May 4, 2018

tomcallahan pushed a commit that referenced this pull request May 4, 2018

Fix docs

d6a91ea

Recently merged #29229 had a doc bug that broke the doc build. This commit fixes.

tomcallahan mentioned this pull request May 11, 2018

Backport get settings API changes to 6.x #30494

Merged

tomcallahan mentioned this pull request May 18, 2018

Change required version for Get Settings transport API changes to 6.4.0 #30706

Merged

tomcallahan removed the backport pending label May 18, 2018

javanna mentioned this pull request May 28, 2018

Add filtering for default index settings #30834

Closed

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

		@@ -45,8 +66,17 @@ public GetSettingsResponse(ImmutableOpenMap<String, Settings> indexToSettings) {

		public String getSetting(String index, String setting) {


		public GetSettingsResponse(ImmutableOpenMap<String, Settings> indexToSettings) {
		private static final ConstructingObjectParser<GetSettingsResponse, Void> PARSER = new ConstructingObjectParser<>(


		public class GetSettingsResponseTests extends AbstractStreamableXContentTestCase<GetSettingsResponse> {

		private final Set<String> indexNames;

Add Get Settings API support to java high-level rest client #29229

Add Get Settings API support to java high-level rest client #29229

Conversation

tomcallahan commented Mar 23, 2018

elasticmachine commented Mar 23, 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

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 23, 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

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

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

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

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