Add support for indices exists to REST high level client #27384

hariso · 2017-11-14T15:39:48Z

This is obviously work in progress, but I opened the PR to ask a few questions:

The small code contributed is a bit confused about the exact phrase to be used "indices exist" or "index exist".
I couldn't find enough info here, w/r to how should the API behave when we check for existence of multiple existence. For now, I assume that if at least index in the list of indices provided is missing, the response is 404. It might be worth thinking about a response where details about each input index individually are included.
Do we need the async version?
I'll add docs soon.

karmi · 2017-11-14T15:39:50Z

Hi @hariso, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

elasticmachine · 2017-11-14T15:39:50Z

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?

hariso · 2017-11-14T15:50:20Z

@karmi Done!

hariso · 2017-11-14T15:50:47Z

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

+        return new IndicesExistsResponse(response.getStatusLine().getStatusCode() == 200);
+    }
+
+    public void existsAsync(IndicesExistsRequest indicesExistsRequest, ActionListener<IndicesExistsResponse> listener, Header... headers) throws IOException {


Async version needed?

hariso · 2017-11-14T15:51:21Z

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

@@ -373,12 +377,12 @@ static Request search(SearchRequest searchRequest) throws IOException {

    static Request searchScroll(SearchScrollRequest searchScrollRequest) throws IOException {
        HttpEntity entity = createEntity(searchScrollRequest, REQUEST_BODY_CONTENT_TYPE);
-        return new Request("GET", "/_search/scroll", Collections.emptyMap(), entity);
+        return new Request("GET", "/_search/scroll", emptyMap(), entity);


I can remove this, if needed.

yes please, I commented above about it, before seeing your question ;)

hariso · 2017-11-14T15:51:47Z

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

@@ -410,10 +414,16 @@ static String endpoint(String... parts) {
     * @return the {@link ContentType}
     */
    @SuppressForbidden(reason = "Only allowed place to convert a XContentType to a ContentType")
-    public static ContentType createContentType(final XContentType xContentType) {
+    public static <Req extends ActionRequest> ContentType createContentType(final XContentType xContentType) {


I will revert.

hariso · 2017-11-14T15:54:43Z

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

@@ -72,7 +72,7 @@ protected static RestHighLevelClient highLevelClient() {

    @FunctionalInterface
    protected interface AsyncMethod<Request, Response> {
-        void execute(Request request, ActionListener<Response> listener, Header... headers);
+        void execute(Request request, ActionListener<Response> listener, Header... headers) throws IOException;


Most probably this is not needed, I'll remove.

hariso · 2017-11-14T15:55:06Z

...c/main/java/org/elasticsearch/action/admin/indices/exists/indices/IndicesExistsResponse.java

+        return null;
+    }
+
+    public static IndicesExistsResponse fromXContent(XContentParser xContentParser) {


I believe none of this is really needed...

hariso · 2017-11-14T16:01:42Z

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

+        );
+    }
+
+    private IndicesExistsResponse parseIndicesExistResp(Response response) {


It might have been better to move this method into IndicesExistsResponse, but Response is not visible from IndicesExistsResponse.

use the already existing convertExistsResponse from RestHighLevelClient, you can see how this is done in the ping method for instance.

javanna

hi @hariso , this looks pretty good, I will remove the WIP label. If you can add docs and address the few comments I left, I will merge this. Thanks!

On your questions: naming is good as-is. We do need the async version I think, which you have already added, so nothing to do there. The behaviour of the API when multiple indices are passed in can be controlled through indices options which are currently not supported in your PR, you'll see a comment about that. Once you support those you just have to return true if 200 and false if 404.

javanna · 2017-12-01T15:12:33Z

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

+        );
+    }
+
+    private IndicesExistsResponse parseIndicesExistResp(Response response) {


use the already existing convertExistsResponse from RestHighLevelClient, you can see how this is done in the ping method for instance.

javanna · 2017-12-01T15:14:01Z

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


 import java.io.IOException;
-import java.util.Collections;
+
+import static java.util.Collections.emptySet;


nit: can you revert the addition of the static import? It affects also other methods that you are not changing and that causes some noise in the PR.

Will do 👍

javanna · 2017-12-01T15:14:06Z

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

@@ -67,6 +69,8 @@
 import java.util.Objects;
 import java.util.StringJoiner;

+import static java.util.Collections.emptyMap;


nit: can you revert the addition of the static import? It affects also other methods that you are not changing and that causes some noise in the PR.

Will do 👍

javanna · 2017-12-01T15:14:26Z

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

@@ -373,12 +377,12 @@ static Request search(SearchRequest searchRequest) throws IOException {

    static Request searchScroll(SearchScrollRequest searchScrollRequest) throws IOException {
        HttpEntity entity = createEntity(searchScrollRequest, REQUEST_BODY_CONTENT_TYPE);
-        return new Request("GET", "/_search/scroll", Collections.emptyMap(), entity);
+        return new Request("GET", "/_search/scroll", emptyMap(), entity);


yes please, I commented above about it, before seeing your question ;)

javanna · 2017-12-01T15:14:48Z

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

@@ -414,6 +418,12 @@ public static ContentType createContentType(final XContentType xContentType) {
        return ContentType.create(xContentType.mediaTypeWithoutParameters(), (Charset) null);
    }

+    static Request indicesExist(IndicesExistsRequest request) {
+        String endpoint = endpoint(request.indices(), Strings.EMPTY_ARRAY, "");
+


can you remove this empty line please?

Will do 👍

javanna · 2017-12-01T15:15:12Z

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

 import org.elasticsearch.rest.RestStatus;

 import java.io.IOException;

 public class IndicesClientIT extends ESRestHighLevelClientTestCase {

+    public void testIndexExists_IndexPresent() throws IOException {


I would prefer not using underscore as part of method names if possible. can you remove this one and the following ones please?

javanna · 2017-12-01T15:30:27Z

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

+        final Request request = Request.indicesExist(indicesExistRequest);
+        assertEquals("/" + index1 + "," + index2, request.getEndpoint());
+        assertEquals("HEAD", request.getMethod());
+    }


I think that one test here is enough. test it with a random number of indices ?

Done! Test is now using up to 10 indices with random names.

javanna · 2017-12-01T15:37:06Z

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

+    static Request indicesExist(IndicesExistsRequest request) {
+        String endpoint = endpoint(request.indices(), Strings.EMPTY_ARRAY, "");
+
+        return new Request(HttpHead.METHOD_NAME, endpoint, emptyMap(), null);


we should add support here for some missing parameters, see https://github.com/elastic/elasticsearch/blob/master/rest-api-spec/src/main/resources/rest-api-spec/api/indices.exists.json .

@javanna I implemented this. Two values I could extract from IndicesExistsRequest I did. Please double check, since I have the feeling I missed something.

javanna · 2017-12-01T15:46:49Z

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

+            listener, emptySet(), headers);
+    }
+
+    public IndicesExistsResponse exists(IndicesExistsRequest indicesExistsRequest, Header... headers) throws IOException {


can you make this final?

I guess you mean the method and not parameters ("this" and not "these"), but I'd like to double check.: )

yes the method.

javanna · 2017-12-01T15:46:58Z

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

+        return new IndicesExistsResponse(response.getStatusLine().getStatusCode() == 200);
+    }
+
+    public void existsAsync(IndicesExistsRequest indicesExistsRequest, ActionListener<IndicesExistsResponse> listener, Header... headers) {


can you make this final?

javanna · 2018-01-16T13:59:52Z

hi @hariso do you think you'll have time to get back to this PR, merge master in and make the requested changes above? Thanks!

hariso · 2018-01-31T14:47:07Z

@javanna Thanks a lot for the feedback! I did a few small refactorings in the tests. I'll now address the other comments and let you once all of that is done.

javanna · 2018-01-31T15:25:27Z

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

@@ -1230,6 +1178,34 @@ private static void setRandomIndicesOptions(Consumer<IndicesOptions> setter, Sup
        }
    }

+    private static void setRandomIncludeDefaults(GetIndexRequest request, Map<String, String> expectedParams) {
+        if (randomBoolean()) {
+            request.includeDefaults(true);


can we also randomize the value, just to test what we do when the value is explicitly set to false? It is the default value but I would prefer to test that codepath too. Same for the other new methods.

javanna · 2018-01-31T15:25:45Z

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

+            request.local(true);
+            expectedParams.put("local", Boolean.TRUE.toString());
+        }
+    }


hariso · 2018-01-31T18:55:40Z

@javanna Another batch of changes is in!
One question related to this ctor:

public GetIndexRequest(StreamInput in) throws IOException {
        super(in);
        int size = in.readVInt();
        features = new Feature[size];
        for (int i = 0; i < size; i++) {
            features[i] = Feature.fromId(in.readByte());
        }
        humanReadable = in.readBoolean();
    }

This is obviously the two new fields, flatSettings and includeDefaults. However, my understanding is that we have added those two fields because of the HLRC only, and this appears to be used by the transport, so I (kinda) intentionally left it out.

javanna · 2018-02-01T15:10:48Z

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

+
+        assertEquals(HttpHead.METHOD_NAME, request.getMethod());
+        assertEquals("/" + String.join(",", indices), request.getEndpoint());
+        assertThat(expectedParams, equalTo(request.getParameters()));


nit: can you assert that request.getEntity is null please?

javanna · 2018-02-01T15:12:40Z

server/src/main/java/org/elasticsearch/action/admin/indices/get/GetIndexRequest.java

@@ -99,6 +98,8 @@ public static Feature fromId(byte id) {
    private static final Feature[] DEFAULT_FEATURES = new Feature[] { Feature.ALIASES, Feature.MAPPINGS, Feature.SETTINGS };
    private Feature[] features = DEFAULT_FEATURES;
    private boolean humanReadable = false;
+    private boolean flatSettings = false;
+    private boolean includeDefaults = false;


the point that you make on these not being serializable is a good one. I think that is a good choice, if we want to make it more explicit we could mark these transient

javanna · 2018-02-01T15:22:47Z

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

+<2> Return result in a format suitable for humans.
+<3> Whether to return all default setting for each of the indices.
+<4> Return settings in flat format.
+<5> Controls how unavailable indices are resolved and how wildcard expressions are expanded.


nit: can you remove the punctuation mark at the end of these notes?

javanna · 2018-02-01T15:22:59Z

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

+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[indices-exists-async]
+--------------------------------------------------
+<1> Called when the execution is successfully completed. The response is provided as an argument.
+<2> Called in case of failure. The raised exception is provided as an argument.


remove the final punctuation marks?

javanna

this is ready @hariso . I left a couple of very minor comments. Docs look good. if you can address the last few comments I will merge this.

hariso · 2018-02-01T20:14:37Z

@javanna Docs and tests updated. I just realized one thing, and that is that it may be good to combine three different tests for Indices Exists API from IndicesClientIT into one, since it looks like that's how it's usually done in ES?

javanna · 2018-02-02T09:10:56Z

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

 include::delete_index.asciidoc[]
+>>>>>>> master


could you fix this? A merge gone wrong I think

include::deleteindex.asciidoc[] should go, it's been replaced by include::delete_index.asciidoc[]

Fixed, thanks for noticing it!

javanna · 2018-02-02T10:25:47Z

thanks a lot @hariso !

Relates to #27205

hariso · 2018-02-02T10:50:05Z

Thank you for all the stuff I learnt here! No wonder Elasticsearch is such a great piece of software!

hariso changed the title ~~High-level REST client: Support for "indices exist"~~ [WIP] High-level REST client: Support for "indices exist" Nov 14, 2017

hariso commented Nov 14, 2017

View reviewed changes

javanna self-assigned this Nov 14, 2017

javanna added :Java High Level REST Client >enhancement WIP labels Nov 14, 2017

javanna self-requested a review November 14, 2017 15:52

hariso commented Nov 14, 2017

View reviewed changes

javanna requested changes Dec 1, 2017

View reviewed changes

javanna changed the title ~~[WIP] High-level REST client: Support for "indices exist"~~ Add support for indices exists to REST high level client Dec 1, 2017

javanna removed the WIP label Dec 1, 2017

javanna reviewed Dec 1, 2017

View reviewed changes

Haris Osmanagić added 7 commits December 1, 2017 17:40

#27205: Indices exist.

a3a2f22

#27205: Indices exist. First tests.

5b6bd87

HLRC: More tests. First functional version.

d05262f

Reverted change (obsolete type declaration)

2e4d42a

Removed useless code.

e57d2ff

Updated docs.

abf2919

Code improvements.

a63880f

javanna mentioned this pull request Dec 6, 2017

Incorrect REST API spec? #27681

Closed

elastic deleted a comment from elasticmachine Jan 16, 2018

Refactored tests.

d665b55

javanna reviewed Jan 31, 2018

View reviewed changes

Haris Osmanagić added 4 commits January 31, 2018 18:47

Improved tests.

3fd5006

more JavaDocs.

fe18f27

Merge branch 'master' into hlrc-indices-exist

1157286

Updated docs. Fixed a test.

9461afb

javanna reviewed Feb 1, 2018

View reviewed changes

javanna approved these changes Feb 1, 2018

View reviewed changes

Haris Osmanagić added 2 commits February 1, 2018 19:41

Updated docs. Added another check to test.

910cc95

Merge branch 'master' into hlrc-indices-exist

8df7602

javanna reviewed Feb 2, 2018

View reviewed changes

Haris Osmanagić added 2 commits February 2, 2018 10:36

Fixed merge error.

870fdfd

Combined tests.

1f57a30

javanna merged commit 897ef45 into elastic:master Feb 2, 2018

javanna added the v7.0.0 label Feb 2, 2018

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

Add support for indices exists to REST high level client (#27384)

934563b

Relates to #27205

javanna added the v6.3.0 label Feb 2, 2018

hariso deleted the hlrc-indices-exist branch February 2, 2018 10:50

jaymode mentioned this pull request Feb 22, 2018

IndicesClient.exists() can not be resolved #28789

Closed

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

Add support for indices exists to REST high level client #27384

Add support for indices exists to REST high level client #27384

Conversation

hariso commented Nov 14, 2017 • edited Loading

karmi commented Nov 14, 2017

elasticmachine commented Nov 14, 2017

hariso commented Nov 14, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jan 16, 2018

hariso commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariso commented Jan 31, 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 left a comment

Choose a reason for hiding this comment

hariso commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Feb 2, 2018

hariso commented Feb 2, 2018

hariso commented Nov 14, 2017 •

edited

Loading