Add support for indices exists to REST high level client by hariso · Pull Request #27384 · elastic/elasticsearch

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

Async version needed?

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

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

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

I will revert.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


+    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

    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::deleteindex.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

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

Copy link
Copy Markdown

Contributor Author

hariso Nov 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will revert.

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 suggested 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

Conversation

hariso commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karmi commented Nov 14, 2017

Uh oh!

elasticmachine commented Nov 14, 2017

Uh oh!

hariso commented Nov 14, 2017

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!

Choose a reason for hiding this comment

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!

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!

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!

javanna commented Jan 16, 2018

Uh oh!

hariso commented Jan 31, 2018

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!

hariso commented Jan 31, 2018

hariso commented Nov 14, 2017 •

edited

Loading