REST high-level client: add support for split and shrink index API #28425

javanna · 2018-01-29T17:00:53Z

Relates to #27205

javanna · 2018-01-29T17:01:51Z

@olcbean maybe you want to have a look too ;)

olcbean

LGMT! I left few comments : mostly nits.

olcbean · 2018-01-29T18:09:43Z

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

@@ -69,7 +67,7 @@ public void testCreateIndex() throws IOException {
            CreateIndexRequest createIndexRequest = new CreateIndexRequest(indexName);

            CreateIndexResponse createIndexResponse =
-                execute(createIndexRequest, highLevelClient().indices()::create, highLevelClient().indices()::createAsync);
+                    execute(createIndexRequest, highLevelClient().indices()::create, highLevelClient().indices()::createAsync);


Thanks for reformatting ;)

olcbean · 2018-01-29T18:14:12Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

        createIndex(name, settings, "");
    }

-    protected void createIndex(String name, Settings settings, String mapping) throws IOException {
+    protected static void createIndex(String name, Settings settings, String mapping) throws IOException {
        assertOK(client().performRequest("PUT", name, Collections.emptyMap(),


Maybe HttpPut.METHOD_NAME instead?

olcbean · 2018-01-29T18:44:06Z

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

+
+    private static void resizeTest(ResizeType resizeType, CheckedFunction<ResizeRequest, Request, IOException> function)
+            throws IOException {
+        ResizeRequest resizeRequest = new ResizeRequest(randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10));


I think it would be better to call toLowerCase as the index should be lowercase ( or generate randomIndicesNames )

olcbean · 2018-01-29T18:49:52Z

test/framework/src/main/java/org/elasticsearch/index/RandomCreateIndexGenerator.java

+     * Returns a random {@link CreateIndexRequest}.
+     */
+    public static CreateIndexRequest randomCreateIndexRequest() throws IOException {
+        String index = randomAlphaOfLength(5);


toLowerCase ?

olcbean · 2018-01-29T19:03:27Z

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

+            if (randomBoolean()) {
+                randomAliases(createIndexRequest);
+            }
+        }


Can this be replaced by randomCreateIndexRequest()? Or maybe removed ?

no, resize supports only a couple of sections compared to create index (aliases and settings only). I think it is better to set only those specifically.

My bad !
But I do not see it being used here. Did you mean to set it as target?

it should be set within the method, that is why we pass in the request, otherwise we'd have to return a List<Alias> and set them one by one as the request doesn't accept a list of alias directly.

I am afraid I am missing something ...
I am not sure why createIndexRequest in initialized but not used in the resizeRequest : I would have expected to see something like :
resizeRequest.setTargetIndex(createIndexRequest) ?
Currently it seems that the targetIndexRequest is always null regardless of the randomization

ops. thanks for being persistent on this :)

olcbean · 2018-01-29T19:16:22Z

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

+            }
+        });
+        // end::shrink-index-execute-async
+    }


@tlrx should we add assertBusy here ( the same as for create and delete ) ? Or here it is not needed ?

I think that this is OK here because the test framework will wait for pending cluster state updates to be processed before executing the next test and resize action is based on cluster state updates.

olcbean · 2018-01-29T20:08:21Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

+        targetIndexRequest.settings().toXContent(builder, params);
+        builder.endObject();
+        builder.startObject(CreateIndexRequest.ALIASES.getPreferredName());
+        for (Alias alias :targetIndexRequest.aliases()) {


nit : missing whitespace

olcbean · 2018-01-29T20:29:40Z

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeResponseTests.java

+    private static ResizeResponse createTestItem() {
+        boolean acknowledged = randomBoolean();
+        boolean shardsAcknowledged = acknowledged && randomBoolean();
+        String index = randomAlphaOfLength(5);


toLowerCase ?

olcbean · 2018-01-29T20:45:50Z

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequestTests.java

+    }
+
+    private static ResizeRequest createTestItem() {
+        ResizeRequest resizeRequest = new ResizeRequest(randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10));


toLowerCase ?

I don't know about this, we don't do it in other tests, not sure that we need it?

Ops : there is not validation of the index names here

olcbean · 2018-01-29T20:51:52Z

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

+                // <2>
+            }
+        });
+        // end::split-index-execute-async


assertBusy : same as above ?

Same answer :)

tlrx

This looks great! I left many comments but most of them are really minor.

tlrx · 2018-01-31T20:27:32Z

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

@@ -484,6 +486,31 @@ static Request existsAlias(GetAliasesRequest getAliasesRequest) {
        return new Request(HttpHead.METHOD_NAME, endpoint, params.getParams(), null);
    }

+    static Request split(ResizeRequest resizeRequest) throws IOException {
+        if (resizeRequest.getResizeType() != ResizeType.SPLIT) {
+            throw new IllegalArgumentException("Wrong resize type for indices.split request: " + resizeRequest.getResizeType());


Nit: can we change the message to something like "Wrong resize type [..] for indices split request"?

tlrx · 2018-01-31T20:32:30Z

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

+        Params params = Params.builder();
+        params.withTimeout(resizeRequest.timeout());
+        params.withMasterTimeout(resizeRequest.masterNodeTimeout());
+        params.withWaitForActiveShards(resizeRequest.getTargetIndexRequest().waitForActiveShards());


Should we check that the target index is not null at this stage?

we already check this in the validate method, which the high level client calls internally.

Yes, I know but I was thinking we could be more defensive. On the other side there's no reason this method would be reused somewhere else so let's keep it like this.

tlrx · 2018-01-31T20:37:27Z

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

+        Map<String, Object> nodes = getAsMap("_nodes");
+        String firstNode = ((Map<String, Object>) nodes.get("nodes")).keySet().iterator().next();
+        createIndex("source", Settings.builder().put("index.number_of_shards", 4).put("index.number_of_replicas", 0).build());
+        updateIndexSettings("source", Settings.builder().put("index.routing.allocation.require._name", firstNode)


Do we expect this test to be executed against something else than a single node cluster?

I am not too sure about this. I was under the impression that it could be either 1 or 2, but not too sure.

I think it's always a single node cluster, but I'm good to keep it like this.

tlrx · 2018-01-31T20:40:14Z

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

+        ResizeResponse resizeResponse = highLevelClient().indices().shrink(resizeRequest);
+        assertTrue(resizeResponse.isAcknowledged());
+        assertTrue(resizeResponse.isShardsAcknowledged());
+        Map<String, Object> indexMetadata = getIndexMetadata("target");


Do you have an objection in using XContentMapValues.extractValue() ?

tlrx · 2018-01-31T20:48:28Z

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

@@ -101,6 +104,9 @@
 import static java.util.Collections.singletonMap;
 import static org.elasticsearch.client.Request.REQUEST_BODY_CONTENT_TYPE;
 import static org.elasticsearch.client.Request.enforceSameContentType;
+import static org.elasticsearch.index.RandomCreateIndexGenerator.randomAliases;


I feel a bit bad about adding another random stuff generator class in the test framework :(

why? we need it in both the client tests and core tests after all. Maybe you have a better name in mind perhaps?

tlrx · 2018-01-31T21:15:25Z

docs/java-rest/high-level/apis/shrink_index.asciidoc

+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[shrink-index-request-waitForActiveShards]
+--------------------------------------------------
+<1> The number of active shard copies to wait for before the shrink index API
+returns a response, as an `int`.


Nit: sometimes there's a point at the end of the line, sometimes not

tlrx · 2018-01-31T21:16:37Z

docs/java-rest/high-level/apis/split_index.asciidoc

+--------------------------------------------------
+<1> The number of active shard copies to wait for before the split index API
+returns a response, as an `int`.
+<2> The number of active shard copies to wait for before  the split index API


Nit: extra space too

tlrx · 2018-01-31T21:22:26Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

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


Nit: could we add some empty lines or anonymous blocks around start/end object to make the object creation easier to read?

tlrx · 2018-01-31T21:28:44Z

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeResponseTests.java

+        assertEquals(resizeResponse.isShardsAcknowledged(), parsedResizeResponse.isShardsAcknowledged());
+        assertEquals(resizeResponse.isAcknowledged(), parsedResizeResponse.isAcknowledged());
+    }
+


nit: extra line

tlrx · 2018-01-31T21:30:40Z

test/framework/src/main/java/org/elasticsearch/index/RandomCreateIndexGenerator.java

+    private RandomCreateIndexGenerator() {}
+
+    /**
+     * Returns a random {@link CreateIndexRequest}.


Maybe add more doc about the randomized parts( aliases, settings, mappings only)?

javanna · 2018-02-01T10:16:19Z

thanks @tlrx I have addressed your comments

tlrx · 2018-02-01T10:28:47Z

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

+     * Shrink Index API on elastic.co</a>
+     */
+    public ResizeResponse shrink(ResizeRequest resizeRequest, Header... headers) throws IOException {
+        return restHighLevelClient.performRequestAndParseEntity(resizeRequest, Request::shrink, ResizeResponse::fromXContent,


Not for this PR but we might want to rename restHighLevelClient to client here

tlrx

LGTM, thanks for the changes

Relates to elastic#27205

…28425) Relates to #27205

javanna added >enhancement review v7.0.0 v6.3.0 labels Jan 29, 2018

javanna requested a review from tlrx January 29, 2018 17:00

javanna mentioned this pull request Jan 29, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

olcbean approved these changes Jan 29, 2018

View reviewed changes

tlrx reviewed Jan 31, 2018

View reviewed changes

javanna force-pushed the enhancement/hl_client_resize branch from 7fbe26a to de78192 Compare February 1, 2018 10:15

tlrx reviewed Feb 1, 2018

View reviewed changes

tlrx approved these changes Feb 1, 2018

View reviewed changes

javanna added 6 commits February 1, 2018 16:35

REST high-level client: add support for split and shrink index API

a000fa1

Relates to elastic#27205

fix compile errors

48108e0

address review comments

4eba77a

fix test bug

bc2b010

address review comments

2e3ba2f

fix failing test

0a1fb6c

javanna force-pushed the enhancement/hl_client_resize branch from 1f91579 to 0a1fb6c Compare February 1, 2018 15:35

javanna merged commit d860971 into elastic:master Feb 1, 2018

javanna added the backport pending label Feb 1, 2018

javanna added a commit that referenced this pull request Feb 2, 2018

REST high-level client: add support for split and shrink index API (#…

f6ce55b

…28425) Relates to #27205

javanna removed the backport pending label Feb 2, 2018

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

REST high-level client: add support for split and shrink index API #28425

REST high-level client: add support for split and shrink index API #28425

Conversation

javanna commented Jan 29, 2018

javanna commented Jan 29, 2018

olcbean 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

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

tlrx 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

javanna Feb 1, 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

javanna commented Feb 1, 2018

Choose a reason for hiding this comment

tlrx left a comment

Choose a reason for hiding this comment

javanna Feb 1, 2018 •

edited

Loading