Added Create Index support to high-level REST client #27351

catalin-ursachi · 2017-11-11T21:24:08Z

Adds one API covered by #27205

elasticmachine · 2017-11-11T21:24:09Z

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?

dnhatn · 2017-11-11T22:06:34Z

@elasticmachine please test this.

catalin-ursachi · 2017-11-12T15:30:36Z

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

+                execute(createIndexRequest, highLevelClient().indices()::createIndex, highLevelClient().indices()::createIndexAsync);
+            assertTrue(createIndexResponse.isAcknowledged());
+
+            assertTrue(indexExists(indexName));


The checks here could be expanded, to check for the alias, mapping and setting, once Get Index is implemented, I suppose; should I add a TODO?

would you mind using the low-level REST client to check this? You can parse the get index api response into a map and compare it to what you 'd expect?

Good shout; alias filter creation was actually failing! Now fixed. ☺

catalin-ursachi · 2017-11-13T15:05:11Z

core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

+            builder.startObject(MAPPINGS.getPreferredName());
+
+            for (Map.Entry<String, String> entry : mappings.entrySet()) {
+                builder.rawField(entry.getKey(), new BytesArray(entry.getValue()), XContentType.JSON);


Seems infelicitous that we store the mappings as JSON-serialized Strings, and hence have to deserialize & reserialize them when making an HTTP request. It would presumably be nicer if we stored mappings as typed objects, akin to Aliases and Settings? Would that be a good idea/outside the scope of this PR?

yea this is annoying but I would not address it as part of this PR. Also, how would you type mappings? Maps of maps?

javanna

thanks a lot @catalin-ursachi and sorry it took me a while to review it. It looks great, I left a few comments but nothing major. I will merge this in as soon as you addressed those.

javanna · 2017-12-01T13:48:23Z

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

+        Params parameters = Params.builder();
+        parameters.withTimeout(createIndexRequest.timeout());
+        parameters.withMasterTimeout(createIndexRequest.masterNodeTimeout());
+        parameters.withWaitForActiveShards(createIndexRequest.waitForActiveShards());


we also support a parameter called updateAllTypes here.

javanna · 2017-12-01T13:51:18Z

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

+                execute(createIndexRequest, highLevelClient().indices()::createIndex, highLevelClient().indices()::createIndexAsync);
+            assertTrue(createIndexResponse.isAcknowledged());
+
+            assertTrue(indexExists(indexName));


would you mind using the low-level REST client to check this? You can parse the get index api response into a map and compare it to what you 'd expect?

javanna · 2017-12-01T13:53:04Z

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


-        assertEquals(200, response.getStatusLine().getStatusCode());
+        assertTrue(response.isAcknowledged());


nit: I wonder if we should leave this method that uses the low-level REST client, given that we call it e.g. when testing delete index, otherwise we end up testing create index also as part of delete index.

Would obviously be trivial to revert to using the low-level client here, but I'd be wary of applying that principle later on - e.g., it we're testing Get Alias, it would be a lot more costly to create the test data using the low-level client, rather than the high-level client Create Alias; and so forth.

javanna · 2017-12-01T13:58:18Z

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

+            // end::delete-index-request-timeout
+            // tag::delete-index-request-masterTimeout
+            request.masterNodeTimeout(TimeValue.timeValueMinutes(1)); // <1>
+            request.timeout("1m"); // <2>


should this one be masterNodeTimeout too?

javanna · 2017-12-01T14:00:18Z

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

+            // end::create-index-request-timeout
+            // tag::create-index-request-masterTimeout
+            request.masterNodeTimeout(TimeValue.timeValueMinutes(1)); // <1>
+            request.timeout("1m"); // <2>


should this also be masterNodeTimeout?

javanna · 2017-12-01T14:32:02Z

docs/java-api/admin/indices/put-mapping.asciidoc

@@ -64,7 +64,7 @@ client.admin().indices().preparePutMapping("twitter")   <1>
        .setSource("{\n" +                              <3>
                "  \"properties\": {\n" +
                "    \"user_name\": {\n" +
-                "      \"type\": \"string\"\n" +
+                "      \"type\": \"text\"\n" +
                "    }\n" +
                "  }\n" +
                "}")


good catches, but may I ask you to send a separate PR for these? I think the problem is that we are not testing these snippets, we should evaluate what to do about it in a different PR.

Yup; will create another PR.

javanna · 2017-12-01T14:33:55Z

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

+/**
+ * Base testcase for indices admin unit testing
+ */
+public abstract class ESIndicesTestCase extends ESTestCase {


can you please add these methods to the CreateIndexRequestTests class where they are used? Let's' postpone the decision on where to put them once they need to be used from some other class too.

javanna · 2017-12-01T14:42:54Z

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

+    public static Alias randomAlias() {
+        Alias alias = new Alias(randomAlphaOfLength(5));
+
+        if (randomBoolean()) {


can you wrap the if in a new if(randomBoolean()) ? I want to make sure that we also test frequently the case where no routing is set.

javanna · 2017-12-01T14:57:26Z

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

+            builder.startObject(randomAlphaOfLength(5));
+
+            if (allowObjectField && randomBoolean()) {
+                randomMappingFields(builder, false);


do you think that we should limit the number of inner levels that we can have?

Do you mean, we shouldn't limit it? I'll change the false to randomBoolean(), so we can go arbitrarily many levels in principle (but with decreasing likelihood).

javanna · 2017-12-01T14:58:27Z

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

+        }
+
+        int numberOfReplicas = randomIntBetween(0, 10);
+        if (numberOfReplicas >= 0) {


why the if? isn't it always true? Maybe you want to have an if(randomBoolean()) that controls whether we set the setting or not?

javanna · 2017-12-01T15:47:09Z

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

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-create-index.html">
+     * Create Index API on elastic.co</a>
+     */
+    public CreateIndexResponse createIndex(CreateIndexRequest createIndexRequest, Header... headers) throws IOException {


can you make it final please?

javanna · 2017-12-01T15:47:13Z

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

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-create-index.html">
+     * Create Index API on elastic.co</a>
+     */
+    public void createIndexAsync(CreateIndexRequest createIndexRequest, ActionListener<CreateIndexResponse> listener, Header... headers) {


can you make it final please?

Same for the delete index methods, I suppose?

catalin-ursachi · 2017-12-02T17:24:11Z

Created #27629 for the 'put mappings' doc update.

javanna

thank you @catalin-ursachi I left some more comments, all minors, PR still looks good.

javanna · 2017-12-04T10:07:28Z

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

 * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/indices.html">Indices API on elastic.co</a>
 */
-public final class IndicesClient {


sorry I got confused with my own previous changes, let's leave this class final, then the final modifier is not necessary on each method. My bad.

javanna · 2017-12-04T10:09:44Z

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

+                execute(createIndexRequest, highLevelClient().indices()::createIndex, highLevelClient().indices()::createIndexAsync);
+            assertTrue(createIndexResponse.isAcknowledged());
+
+            assertTrue(indexExists(indexName));


javanna · 2017-12-04T10:10:55Z

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

+                execute(createIndexRequest, highLevelClient().indices()::createIndex, highLevelClient().indices()::createIndexAsync);
+            assertTrue(createIndexResponse.isAcknowledged());
+
+            Map<?, ?> indexMetaData = getIndexMetadata(indexName);


Can you use Map<String, Object> instead?

If I try to cast:

Map<String, Object> aliasData = (Map<String, Object>) aliasesData.get("alias_name");

IntelliJ highlights it with a warning that it constitutes an "Unchecked cast" (since type erasure makes casting to a specific type of Map meaningless anyway).
Should I ignore the warning and make the above change regardless?

you can add SuppressWarnings("unchecked") to the whole method like we do in other places.

javanna · 2017-12-04T10:12:26Z

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

+        setRandomWaitForActiveShards(createIndexRequest::waitForActiveShards, expectedParams);
+
+        boolean updateAllTypes = randomBoolean();
+        createIndexRequest.updateAllTypes(updateAllTypes);


can you set it only randomly so we also exercise the default value for it?

javanna · 2017-12-04T11:03:37Z

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequestTests.java

+
+        for (Alias expectedAlias : expected) {
+            for (Alias actualAlias : actual) {
+                if (expectedAlias.equals(actualAlias)) {


I guess we will never get here if they are not equal?

Yeah; we check above that the two sets are equal; but Alias.equals returns true if the compared aliases have the same name. So, the sets will be equal iff they contain sets of aliases with the same names. At this stage, then, we check that aliases with the same name also have the same parameters.

Maybe we should also check routing params, here?

I see why you do that, it would be good to also check routing. It is odd that Alias#equals only looks at the name, not sure why but I wouldn't change that at the moment. I just meant that the if is redundant, isn't the equality check already part of the above assertion, which will make the test fail in case it's not satisfied?

Ah, no; here, we're iterating over the two sets, but we only want to compare matching aliases. Without the if, we'd be comparing each alias from expected to every alias from actual - which would of course fail.

right! silly me ;) mind leaving a comment saying that we do this because Alias#equals only looks at name?

javanna · 2017-12-04T11:03:42Z

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequestTests.java

@@ -113,6 +135,18 @@ private void assertMappingsEqual(Map<String, String> expected, Map<String, Strin
        }
    }

+    private void assertAliasesEqual(Set<Alias> expected, Set<Alias> actual) throws IOException {


make it static?

javanna · 2017-12-04T11:04:27Z

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequestTests.java

+        builder.endObject();
+    }
+
+    public static Alias randomAlias() {


do these new methods need to be public? If not, let's keep them private and eventually make them public once we need them elsewhere?

javanna · 2017-12-04T11:07:04Z

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequestTests.java

@@ -137,4 +171,72 @@ private static CreateIndexRequest createTestItem() throws IOException {

        return request;
    }
+
+    public static Settings randomSettings() {


maybe rename to randomIndexSettings ?

javanna · 2017-12-04T11:09:31Z

core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequestTests.java

+            builder.startObject(randomAlphaOfLength(5));
+
+            if (allowObjectField && randomBoolean()) {
+                randomMappingFields(builder, randomBoolean());


sorry, I see only now that you were already limiting the number of nested objects. My concern was to not end up with too many levels of nested objects. I think passing in false was good here, we do not need complicated mappings in this test.

javanna

thank you @catalin-ursachi I left some more comments, all minors, PR still looks good.

javanna · 2017-12-06T11:46:28Z

test this please

javanna · 2017-12-06T13:29:18Z

retest this please

catalin-ursachi · 2017-12-06T16:58:06Z

None of the failures seemed related to the changes; will the merge with master have helped, or is the build just generally flaky?

javanna · 2017-12-06T21:01:01Z

don't worry @catalin-ursachi this is ready to be merged, failures are totally unrelated, we just were unlucky.

javanna · 2017-12-07T10:40:17Z

thanks again @catalin-ursachi

Relates to #27205

Added Create Index support to high-level REST client

3a9c8cb

dnhatn requested a review from javanna November 11, 2017 22:06

dnhatn added the :Java High Level REST Client label Nov 11, 2017

catalin-ursachi added 2 commits November 11, 2017 23:20

Added Create Index API docs

6515c8b

Use ParseFields throughout

00ce36c

catalin-ursachi commented Nov 12, 2017

View reviewed changes

catalin-ursachi commented Nov 13, 2017

View reviewed changes

javanna self-assigned this Nov 13, 2017

Fixed typo

472a07e

javanna approved these changes Dec 1, 2017

View reviewed changes

javanna reviewed Dec 1, 2017

View reviewed changes

Fixed Alias filters; mark-ups

d121f6e

catalin-ursachi mentioned this pull request Dec 2, 2017

Updated "string" datatype in docs & tests to "text" #27629

Merged

Merge branch 'master' into 27205-create-index

fa3c2af

javanna approved these changes Dec 4, 2017

View reviewed changes

catalin-ursachi added 2 commits December 4, 2017 12:31

Mark-ups

9c227f5

Added Alias assertion explanation

b9d1137

Merge remote-tracking branch 'origin/master' into 27205-create-index

8193fd5

javanna merged commit f823cea into elastic:master Dec 7, 2017

javanna changed the title ~~Added Create Index support to high-level REST client (#27205)~~ Added Create Index support to high-level REST client Dec 7, 2017

elastic deleted a comment from elasticmachine Dec 7, 2017

javanna added v7.0.0 >enhancement v6.2.0 backport pending labels Dec 7, 2017

javanna pushed a commit that referenced this pull request Dec 10, 2017

Added Create Index support to high-level REST client (#27351)

c20cbe5

Relates to #27205

javanna removed the backport pending label Dec 10, 2017

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


		assertEquals(200, response.getStatusLine().getStatusCode());
		assertTrue(response.isAcknowledged());

Added Create Index support to high-level REST client #27351

Added Create Index support to high-level REST client #27351

Conversation

catalin-ursachi commented Nov 11, 2017 • edited Loading

elasticmachine commented Nov 11, 2017

dnhatn commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

catalin-ursachi commented Dec 2, 2017

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

catalin-ursachi Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

javanna commented Dec 6, 2017

javanna commented Dec 6, 2017

catalin-ursachi commented Dec 6, 2017

javanna commented Dec 6, 2017

javanna commented Dec 7, 2017

catalin-ursachi commented Nov 11, 2017 •

edited

Loading

catalin-ursachi Dec 4, 2017 •

edited

Loading