REST high-level client: add support for exists alias#28332
REST high-level client: add support for exists alias#28332javanna merged 5 commits intoelastic:masterfrom
Conversation
|
@olcbean given that you are working on adding support for update aliases, maybe you want to review this one? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yea probably, I will sync with @tlrx who has made that change. thanks for catching this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok I will make that change to open/close separately then
olcbean
left a comment
There was a problem hiding this comment.
LGTM. Left a few minor nits
There was a problem hiding this comment.
Can we change it to emptySet() as below?
There was a problem hiding this comment.
Thanks! Makes things easier :) Could you do the same for createIndex and deleteIndex?
There was a problem hiding this comment.
There is a utility createIndex. Should we use it instead?
There was a problem hiding this comment.
Should this be randomIndicesNames(1, 5) instead?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
I though this for GET rather than for HEAD? In the specs the alias is set to required. Am I missing something?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
you are right, thanks for pointing this out.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
With the last commit I addressed all the comments that I got till now. |
There was a problem hiding this comment.
Nit: could it be renamed to expectedEndpoint?
|
retest this please |
edd0e06 to
0f32d21
Compare
* 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) ...
Relates to #27205