Add Open Index API to the high level REST client by olcbean · Pull Request #27574 · elastic/elasticsearch

olcbean · 2017-11-28T20:28:38Z

Add _open to the high level REST client

Relates to #27205

elasticmachine · 2017-11-28T20:28:39Z

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?

DaveCTurner · 2017-11-28T21:14:28Z

@elasticmachine please test this

javanna

thanks a lot @olcbean I left a few comments, looks good though. I will merge this once you addressed those.

javanna · 2017-12-01T19:56:30Z

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

+     */
+    public final void openIndexAsync(OpenIndexRequest openIndexRequest, ActionListener<OpenIndexResponse> listener, Header... headers) {
+        performRequestAsyncAndParseEntity(openIndexRequest, Request::openIndex, OpenIndexResponse::fromXContent, listener,
+                Collections.singleton(404), headers);


we need these only in IndicesClient, so that users can do client.indices().openIndex() . No need to add them here too.

javanna · 2017-12-01T19:57:47Z

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

+        OpenIndexRequest openIndexRequest = new OpenIndexRequest(indexName);
+        OpenIndexResponse openIndexResponse = execute(openIndexRequest, highLevelClient().indices()::openIndex,
+                highLevelClient().indices()::openIndexAsync);
+        assertTrue(openIndexResponse.isAcknowledged());


maybe it would be good to close the index first using the low-level REST client, then verify that opening it actually does something?

javanna · 2017-12-01T19:59:06Z

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

+
+        ElasticsearchException exception = expectThrows(ElasticsearchException.class,
+                () -> execute(openIndexRequest, highLevelClient().indices()::openIndex, highLevelClient().indices()::openIndexAsync));
+        assertEquals(RestStatus.NOT_FOUND, exception.status());


maybe also test the API against multiple indices?

javanna · 2017-12-01T19:59:48Z

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

+        Map<String, String> expectedParams = new HashMap<>();
+        setRandomTimeout(openIndexRequest::timeout, AcknowledgedRequest.DEFAULT_ACK_TIMEOUT, expectedParams);
+        setRandomMasterTimeout(openIndexRequest, expectedParams);
+        setRandomIndicesOptions(openIndexRequest::indicesOptions, openIndexRequest::indicesOptions, expectedParams);


can we also add the set wait for active shards ?

javanna · 2017-12-01T20:02:08Z

core/src/test/java/org/elasticsearch/action/admin/indices/open/OpenIndexResponseTests.java

+        boolean humanReadable = randomBoolean();
+        final XContentType xContentType = randomFrom(XContentType.values());
+        BytesReference originalBytes = toShuffledXContent(openIndexResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
+


can you also add here an insertRandomFields step to verify forward compatibility (as we ignore any unknown fields rather than throwing errors)

javanna · 2017-12-01T20:02:45Z

core/src/test/java/org/elasticsearch/action/admin/indices/open/OpenIndexResponseTests.java

+        final XContentType xContentType = randomFrom(XContentType.values());
+        BytesReference originalBytes = toShuffledXContent(openIndexResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
+
+        OpenIndexResponse parsedCreateIndexResponse;


rename this variable?

olcbean · 2017-12-03T21:45:49Z

@javanna thanks for the review. Ready for a second round ;)

javanna · 2017-12-04T11:24:25Z

test this please

javanna

thanks @olcbean I left a few more comments, nothing major. The important bit is removing the new methods from RestHighLevelClient.

javanna · 2017-12-04T11:17:43Z

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

     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-get.html">Get API on elastic.co</a>
     */
-    public void getAsync(GetRequest getRequest, ActionListener<GetResponse> listener, Header... headers) {
+    public final void getAsync(GetRequest getRequest, ActionListener<GetResponse> listener, Header... headers) {


thank you, seems like I left this out :)

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

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

+    public final void openIndexAsync(OpenIndexRequest openIndexRequest, ActionListener<OpenIndexResponse> listener, Header... headers) {
+        performRequestAsyncAndParseEntity(openIndexRequest, Request::openIndex, OpenIndexResponse::fromXContent, listener, emptySet(),
+                headers);
+    }


we don't need these two methods, they are already in IndicesClient. Can you remove them?

javanna · 2017-12-04T11:19:50Z

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

+        return randomIndices(0, 5);
+    }
+
+    private String[] randomIndices(int minIndicesNum, int maxIndicesNum) {


make this static?

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

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

+        assertEquals(RestStatus.NOT_FOUND, exception.status());
+    }
+
+    private String[] randomIndices() {


maybe we can remove this method and always call the other one?

javanna · 2017-12-04T11:20:53Z

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

+        assertTrue(openIndexResponse.isAcknowledged());
+
+        for (String index : indices) {
+            client().performRequest("GET", index + "/_search");


check that 200 was returned?

javanna · 2017-12-04T11:22:20Z

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

+        OpenIndexRequest openIndexRequest = new OpenIndexRequest(nonExistentIndices);
+        ElasticsearchException exception = expectThrows(ElasticsearchException.class,
+                () -> execute(openIndexRequest, highLevelClient().indices()::openIndex, highLevelClient().indices()::openIndexAsync));
+        assertEquals(RestStatus.NOT_FOUND, exception.status());


would you mind checking also that with IndicesOptions.lenientExpandOpen we don't get back an error?

Clean up

olcbean · 2017-12-04T12:52:30Z

@javanna my bad! Ready for another round ;)

javanna

LGTM thanks @olcbean

javanna · 2017-12-06T11:55:34Z

test this please

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

retest this please

javanna · 2017-12-07T10:57:14Z

heya @olcbean I took the liberty to merge master in as there were conflicts and push to your remote. Running tests one last time and then merging. Thanks a lot!

javanna · 2017-12-07T10:57:29Z

retest this please

olcbean · 2017-12-07T13:51:16Z

Thanks @javanna!
I will continue with _close ( if nobody is working on it )

javanna · 2017-12-07T17:15:48Z

sounds great @olcbean thank you!

* master: (414 commits) Set ACK timeout on indices service test Implement byte array reusage in `NioTransport` (elastic#27696) [TEST] Remove leftover ES temp directories before Vagrant tests (elastic#27722) Cleanup split strings by comma method Remove unused import from AliasResolveRoutingIT Add read timeouts to http module (elastic#27713) Fix routing with leading or trailing whitespace remove await fix from FullClusterRestartIT.testRecovery Add missing 's' to tmpdir name (elastic#27721) [Issue-27716]: CONTRIBUTING.md IntelliJ configurations settings are confusing. (elastic#27717) [TEST] Now actually wait for merges Test out of order delivery of append only index and retry with an intermediate delete [TEST] remove code duplications in RequestTests [Tests] Add test for GeoShapeFieldType#setStrategyName (elastic#27703) Remove unused *Commit* classes (elastic#27714) Add test for writer operation buffer accounting (elastic#27707) [TEST] Wait for merging to complete before testing breaker Add Open Index API to the high level REST client (elastic#27574) Correcting some minor typos in comments Add unreleased v5.6.6 version ...

Add _open to the high level REST client Relates to #27205

Add open index api to the high level rest client

9e25962

DaveCTurner requested a review from javanna November 28, 2017 21:13

DaveCTurner added v6.2.0 v7.0.0 :Java High Level REST Client labels Nov 28, 2017

javanna self-assigned this Dec 1, 2017

javanna added the >enhancement label Dec 1, 2017

javanna suggested changes Dec 1, 2017

View reviewed changes

olcbean added 2 commits December 3, 2017 20:44

Addressing reviewers remarks

cbfaf34

Setting getAsync to final

ae7ede9

javanna suggested changes Dec 4, 2017

View reviewed changes

Added tests for strictExpandOpen and lenientExpandOpen

c3f85ee

Clean up

javanna approved these changes Dec 6, 2017

View reviewed changes

Merge branch 'master' into hlrc_open_index

f63d4a3

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

javanna added the backport pending label Dec 7, 2017

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

Add Open Index API to the high level REST client (#27574)

80be19a

Add _open to the high level REST client Relates to #27205

javanna removed the backport pending label Dec 10, 2017

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

Conversation

olcbean commented Nov 28, 2017

Uh oh!

elasticmachine commented Nov 28, 2017

Uh oh!

DaveCTurner commented Nov 28, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olcbean commented Dec 3, 2017

Uh oh!

javanna commented Dec 4, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olcbean commented Dec 4, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

javanna commented Dec 6, 2017

Uh oh!

javanna commented Dec 6, 2017

Uh oh!

javanna commented Dec 7, 2017

Uh oh!

javanna commented Dec 7, 2017

Uh oh!

olcbean commented Dec 7, 2017

Uh oh!

javanna commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants