REST high-level client: add support for exists alias #28332

javanna · 2018-01-23T10:50:38Z

Relates to #27205

javanna · 2018-01-23T10:51:24Z

@olcbean given that you are working on adding support for update aliases, maybe you want to review this one?

catalin-ursachi · 2018-01-23T15:35:37Z

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

+            // end::exists-alias-execute
+            assertTrue(exists);
+
+            // tag::exists-alias-execute-async


Shouldn't the change made by #27899 be applied to Exists Alias (and indeed Open Index and Close Index, which were presumably merged later) as well?

yea probably, I will sync with @tlrx who has made that change. thanks for catching this.

That would be better, yes

I am wondering though: the original problem was that we were creating/deleting indices asynchronously, hence we would have to wait for such operation to be completed.
Do we have to wait for any other async operation like open/close index etc.? I would say no but maybe @tlrx can confirm.

I don't think it's important for the Exist Alias API as it does not modify the cluster state or requires a lock on shards, but for the open/close ones it might be necessary yes.

ok I will make that change to open/close separately then

olcbean

LGTM. Left a few minor nits

olcbean · 2018-01-23T14:42:10Z

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

@@ -157,4 +160,26 @@ public void closeAsync(CloseIndexRequest closeIndexRequest, ActionListener<Close
        restHighLevelClient.performRequestAsyncAndParseEntity(closeIndexRequest, Request::closeIndex, CloseIndexResponse::fromXContent,
                listener, Collections.emptySet(), headers);


Can we change it to emptySet() as below?

olcbean · 2018-01-23T14:44:26Z

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

@@ -156,7 +157,7 @@ static Request openIndex(OpenIndexRequest openIndexRequest) {
    }

    static Request closeIndex(CloseIndexRequest closeIndexRequest) {
-        String endpoint = endpoint(closeIndexRequest.indices(), Strings.EMPTY_ARRAY, "_close");
+        String endpoint = endpoint(closeIndexRequest.indices(), "_close");


Thanks! Makes things easier :) Could you do the same for createIndex and deleteIndex?

olcbean · 2018-01-23T14:51:06Z

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

+        GetAliasesRequest getAliasesRequest = new GetAliasesRequest("alias");
+        assertFalse(execute(getAliasesRequest, highLevelClient().indices()::existsAlias, highLevelClient().indices()::existsAliasAsync));
+
+        client().performRequest("PUT", "/index");


There is a utility createIndex. Should we use it instead?

olcbean · 2018-01-23T15:35:51Z

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

+        GetAliasesRequest getAliasesRequest = new GetAliasesRequest();
+        String[] indices = randomIndicesNames(0, 5);
+        getAliasesRequest.indices(indices);
+        String[] aliases = randomIndicesNames(0, 5);


Should this be randomIndicesNames(1, 5) instead?

no the idea was to also test the case where the aliases array is empty, that is supported and should be interpreted as "all aliases"

I though this for GET rather than for HEAD? In the specs the alias is set to required. Am I missing something?

the SPEC may be outdated. the code and behaviour is the same for HEAD or GET in the server side, we just don't return a response body when HEAD is used.

Oh, I see now.. My point was for the corner case when there is no alias and no index, then only GET _alias is supported, isn't it?

you are right, thanks for pointing this out.

olcbean · 2018-01-23T15:46:25Z

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

+    static String endpoint(String[] indices, String endpoint) {
+        return buildEndpoint(String.join(",", indices), endpoint);
+    }
+
    static String endpoint(String[] indices, String[] types, String endpoint) {


AFAIK in 7.0 multiple types are no longer supported.
I am not really sure it is nice to offer the possibility to use multiple types. What do you think?

that is the plan, but we are not there yet. What we currently do in the client is aligned with what the API supports. Once the API will change, it will be time to also update the client.

javanna · 2018-01-23T17:18:36Z

With the last commit I addressed all the comments that I got till now.

tlrx

LGTM, left some minor comments

tlrx · 2018-01-24T09:57:02Z

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

+        setRandomIndicesOptions(getAliasesRequest::indicesOptions, getAliasesRequest::indicesOptions, expectedParams);
+
+        Request request = Request.existsAlias(getAliasesRequest);
+        StringJoiner endpoint = new StringJoiner("/", "/", "");


Nit: could it be renamed to expectedEndpoint?

tlrx · 2018-01-24T10:00:22Z

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

+--------------------------------------------------
+include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[exists-alias-request-alias]
+--------------------------------------------------
+<1> One or more aliases  to look for


Nit: extra space

tlrx · 2018-01-24T10:02:27Z

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

+==== Exists Alias Response
+
+The Exists Alias API returns a `boolean` that indicates whether the provided
+alias (or aliases) were found or not.


Nit: was instead or were?

javanna · 2018-01-25T10:27:00Z

retest this please

Relates to #27205

* master: (23 commits) Update Netty to 4.1.16.Final (elastic#28345) Fix peer recovery flushing loop (elastic#28350) REST high-level client: add support for exists alias (elastic#28332) REST high-level client: move to POST when calling API to retrieve which support request body (elastic#28342) Add Indices Aliases API to the high level REST client (elastic#27876) Java Api clean up: remove deprecated `isShardsAcked` (elastic#28311) [Docs] Fix explanation for `from` and `size` example (elastic#28320) Adapt bwc version after backport elastic#28358 Always return the after_key in composite aggregation response (elastic#28358) Adds test name to MockPageCacheRecycler exception (elastic#28359) Adds a note in the `terms` aggregation docs regarding pagination (elastic#28360) [Test] Fix DiscoveryNodesTests.testDeltas() (elastic#28361) Update packaging tests to work with meta plugins (elastic#28336) Remove Painless Type from MethodWriter in favor of Java Class. (elastic#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (elastic#28348) Reindex: Shore up rethrottle test Only assert single commit iff index created on 6.2 isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (elastic#27766) Fix GeoDistance query example (elastic#28355) ...

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

javanna mentioned this pull request Jan 23, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna requested a review from tlrx January 23, 2018 15:07

catalin-ursachi reviewed Jan 23, 2018

View reviewed changes

olcbean approved these changes Jan 23, 2018

View reviewed changes

tlrx approved these changes Jan 24, 2018

View reviewed changes

javanna added 5 commits January 25, 2018 14:37

REST high-level client: add support for exists alias

38dbb8d

address review comments

5cb6e7d

remove assertBusy blocks

dcf0994

address review comments

55e87d8

add protection for existsAlias with no index and no alias

0f32d21

javanna force-pushed the enhancement/hl_client_exists_alias branch from edd0e06 to 0f32d21 Compare January 25, 2018 13:52

javanna merged commit fd66c94 into elastic:master Jan 25, 2018

javanna added the backport pending label Jan 25, 2018

javanna added a commit that referenced this pull request Jan 25, 2018

REST high-level client: add support for exists alias (#28332)

37dfa50

Relates to #27205

javanna removed the backport pending label Jan 25, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST high-level client: add support for exists alias #28332

REST high-level client: add support for exists alias #28332

javanna commented Jan 23, 2018

javanna commented Jan 23, 2018

catalin-ursachi Jan 23, 2018

javanna Jan 23, 2018

tlrx Jan 23, 2018

javanna Jan 24, 2018

tlrx Jan 24, 2018

javanna Jan 24, 2018

olcbean left a comment

olcbean Jan 23, 2018

olcbean Jan 23, 2018

javanna Jan 23, 2018

olcbean Jan 23, 2018

olcbean Jan 23, 2018

javanna Jan 23, 2018

olcbean Jan 24, 2018

javanna Jan 24, 2018

olcbean Jan 24, 2018

javanna Jan 25, 2018

olcbean Jan 23, 2018

javanna Jan 23, 2018

javanna commented Jan 23, 2018

tlrx left a comment

tlrx Jan 24, 2018

tlrx Jan 24, 2018

tlrx Jan 24, 2018

javanna commented Jan 25, 2018

		@@ -157,4 +160,26 @@ public void closeAsync(CloseIndexRequest closeIndexRequest, ActionListener<Close
		restHighLevelClient.performRequestAsyncAndParseEntity(closeIndexRequest, Request::closeIndex, CloseIndexResponse::fromXContent,
		listener, Collections.emptySet(), headers);

REST high-level client: add support for exists alias #28332

REST high-level client: add support for exists alias #28332

Conversation

javanna commented Jan 23, 2018

javanna commented Jan 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

javanna commented Jan 23, 2018

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

javanna commented Jan 25, 2018