REST high-level client: add support for split and shrink index API by javanna · Pull Request #28425 · elastic/elasticsearch

javanna · 2018-01-29T17:00:53Z

Relates to #27205

javanna · 2018-01-29T17:01:51Z

@olcbean maybe you want to have a look too ;)

olcbean

LGMT! I left few comments : mostly nits.

olcbean · 2018-01-29T18:09:43Z

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

Thanks for reformatting ;)

olcbean · 2018-01-29T18:14:12Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

Maybe HttpPut.METHOD_NAME instead?

olcbean · 2018-01-29T18:44:06Z

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

I think it would be better to call toLowerCase as the index should be lowercase ( or generate randomIndicesNames )

olcbean · 2018-01-29T18:49:52Z

test/framework/src/main/java/org/elasticsearch/index/RandomCreateIndexGenerator.java

toLowerCase ?

olcbean · 2018-01-29T19:03:27Z

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

Can this be replaced by randomCreateIndexRequest()? Or maybe removed ?

no, resize supports only a couple of sections compared to create index (aliases and settings only). I think it is better to set only those specifically.

My bad !
But I do not see it being used here. Did you mean to set it as target?

it should be set within the method, that is why we pass in the request, otherwise we'd have to return a List<Alias> and set them one by one as the request doesn't accept a list of alias directly.

I am afraid I am missing something ...
I am not sure why createIndexRequest in initialized but not used in the resizeRequest : I would have expected to see something like :
resizeRequest.setTargetIndex(createIndexRequest) ?
Currently it seems that the targetIndexRequest is always null regardless of the randomization

ops. thanks for being persistent on this :)

olcbean · 2018-01-29T19:16:22Z

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

@tlrx should we add assertBusy here ( the same as for create and delete ) ? Or here it is not needed ?

I think that this is OK here because the test framework will wait for pending cluster state updates to be processed before executing the next test and resize action is based on cluster state updates.

olcbean · 2018-01-29T20:08:21Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

nit : missing whitespace

olcbean · 2018-01-29T20:29:40Z

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeResponseTests.java

toLowerCase ?

olcbean · 2018-01-29T20:45:50Z

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequestTests.java

toLowerCase ?

I don't know about this, we don't do it in other tests, not sure that we need it?

Ops : there is not validation of the index names here

olcbean · 2018-01-29T20:51:52Z

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

assertBusy : same as above ?

Same answer :)

tlrx

This looks great! I left many comments but most of them are really minor.

tlrx · 2018-01-31T20:27:32Z

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

Nit: can we change the message to something like "Wrong resize type [..] for indices split request"?

tlrx · 2018-01-31T20:32:30Z

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

Should we check that the target index is not null at this stage?

we already check this in the validate method, which the high level client calls internally.

Yes, I know but I was thinking we could be more defensive. On the other side there's no reason this method would be reused somewhere else so let's keep it like this.

tlrx · 2018-01-31T20:37:27Z

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

Do we expect this test to be executed against something else than a single node cluster?

I am not too sure about this. I was under the impression that it could be either 1 or 2, but not too sure.

I think it's always a single node cluster, but I'm good to keep it like this.

tlrx · 2018-01-31T20:40:14Z

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

Do you have an objection in using XContentMapValues.extractValue() ?

tlrx · 2018-01-31T20:48:28Z

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

I feel a bit bad about adding another random stuff generator class in the test framework :(

why? we need it in both the client tests and core tests after all. Maybe you have a better name in mind perhaps?

tlrx · 2018-01-31T21:15:25Z

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

Nit: sometimes there's a point at the end of the line, sometimes not

tlrx · 2018-01-31T21:16:37Z

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

Nit: extra space too

tlrx · 2018-01-31T21:22:26Z

server/src/main/java/org/elasticsearch/action/admin/indices/shrink/ResizeRequest.java

Nit: could we add some empty lines or anonymous blocks around start/end object to make the object creation easier to read?

tlrx · 2018-01-31T21:28:44Z

server/src/test/java/org/elasticsearch/action/admin/indices/shrink/ResizeResponseTests.java

nit: extra line

tlrx · 2018-01-31T21:30:40Z

test/framework/src/main/java/org/elasticsearch/index/RandomCreateIndexGenerator.java

Maybe add more doc about the randomized parts( aliases, settings, mappings only)?

javanna · 2018-02-01T10:16:19Z

thanks @tlrx I have addressed your comments

tlrx · 2018-02-01T10:28:47Z

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

Not for this PR but we might want to rename restHighLevelClient to client here

tlrx

LGTM, thanks for the changes

Relates to elastic#27205

…28425) Relates to #27205

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

javanna requested a review from tlrx January 29, 2018 17:00

javanna mentioned this pull request Jan 29, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

olcbean approved these changes Jan 29, 2018

View reviewed changes

tlrx reviewed Jan 31, 2018

View reviewed changes

javanna force-pushed the enhancement/hl_client_resize branch from 7fbe26a to de78192 Compare February 1, 2018 10:15

tlrx reviewed Feb 1, 2018

View reviewed changes

tlrx approved these changes Feb 1, 2018

View reviewed changes

javanna added 6 commits February 1, 2018 16:35

REST high-level client: add support for split and shrink index API

a000fa1

Relates to elastic#27205

fix compile errors

48108e0

address review comments

4eba77a

fix test bug

bc2b010

address review comments

2e3ba2f

fix failing test

0a1fb6c

javanna force-pushed the enhancement/hl_client_resize branch from 1f91579 to 0a1fb6c Compare February 1, 2018 15:35

javanna merged commit d860971 into elastic:master Feb 1, 2018

javanna added the backport pending label Feb 1, 2018

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

REST high-level client: add support for split and shrink index API (#…

f6ce55b

…28425) Relates to #27205

javanna removed the backport pending label Feb 2, 2018

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

Conversation

javanna commented Jan 29, 2018

Uh oh!

javanna commented Jan 29, 2018

Uh oh!

olcbean 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!

tlrx 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!

javanna Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

javanna Feb 1, 2018 •

edited

Loading