Add Index API to High Level Rest Client by tlrx · Pull Request #23040 · elastic/elasticsearch

tlrx · 2017-02-08T09:25:55Z

This commit adds support for the Index API in the High Level Rest Client.

It changes few things in the way request's parameters map is created in the high level client and changea bit RefreshPolicy in core.

javanna · 2017-02-08T16:20:32Z

core/src/main/java/org/elasticsearch/action/support/WriteRequest.java

can you make this change separately please? It will need to be backported , while this PR targets master only and should only change stuff under rest-high-level.

Sure - this part has been removed from the PR, I'll create another PR for the WriteRequest changes.

PR created here: #23106

tlrx · 2017-02-10T11:14:32Z

Code rebased and updated

cbuescher

@tlrx I left a few questions and minor remarks, some of it just ideas so from my point of view this is very close.

cbuescher · 2017-02-10T11:57:15Z

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

This might be paranoid, but in case some other previous call to the builder set this to some other value, can we remove that value from the map in case it is set to NONE later?

Scratch that, I see this is private and only used internally, and the request only has one of each parameters, so duplicate calls are unlikely. Just me being paranoid.

Paranoid is good sometimes :) You're right, I'll improve that in order to be remove the value in case of NONE.

shall we rather have a check in putParam for the return value and assert that the value should never be there. If it is it's our bug right?

cbuescher · 2017-02-10T12:24:19Z

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

Maybe we can replace calls of this helper with the new Endpoint class? Might get a tad bit longer in the end, so I'm fine either way.

Yes we can. I'm also waiting for Luca opinion on those Endpoint/Params classes, but if he likes it I'll move those as well.

feel free to unify this with what you do with index. I like these changes overall, I left one comment on the endpoint part which may help improving things a bit, that's it.

cbuescher · 2017-02-10T12:26:51Z

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

Nit: maybe move this up next to the other methods (ping/exists/get) that output new Requests.

cbuescher · 2017-02-10T12:30:49Z

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

Previous test tries all content Types, this one random choice only. Any reason for this?

Not, it's a leftover in the previous test, XContent type must be chosen randomly at the beginning of all tests. I changed that.

cbuescher · 2017-02-10T12:35:03Z

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

Would not setting the version at all be valid here? If so, maybe also test this (e.g. rarely?)

cbuescher · 2017-02-10T12:38:19Z

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

I don't know if it is compatible, but maybe you can use RandomObjects#randomSource(Random random) here instead?

javanna

left a few comments but no blockers, LGTM

javanna · 2017-02-10T20:58:34Z

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

given that we build a url, do we really need two constants for the same thing?

I changed to

private static final String DELIMITER = "/"; private String prefix = DELIMITER; private String suffix = "";

and also added a withPrefix method that was missing. I find it useful for building endpoints for Snapshot/Restore: /_snapshot could be defined as a prefix. Just a matter of taste

Actually, I removed all this stuff in favor of a simple concatenating method helper.

javanna · 2017-02-10T21:00:17Z

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

shall we make it accept a varargs instead and make this a single line?

javanna · 2017-02-10T21:05:29Z

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

given that the suffix is also known before we go and provide index, type, and id... (even in case of e.g. _search) I wonder if we can get rid of the list, and just provide parts and suffix as constructor argument, then convert it into string straightaway. Not even sure we need a builder for this.

Good point, let's do this.

javanna · 2017-02-10T21:07:49Z

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

shall we rather have a check in putParam for the return value and assert that the value should never be there. If it is it's our bug right?

javanna · 2017-02-10T21:10:12Z

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

very nice tests, thanks!

javanna · 2017-02-10T21:11:28Z

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

Use HttpPut.METHOD_NAME maybe? or define our own constants?

javanna · 2017-02-10T21:14:01Z

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

feel free to unify this with what you do with index. I like these changes overall, I left one comment on the endpoint part which may help improving things a bit, that's it.

This commit adds support for the Index API in the High Level Rest Client.

tlrx · 2017-02-13T11:30:39Z

@cbuescher @javanna Thanks for the reviews. I updated the code according to your comments. I also had to rebase in order to use #23106

cbuescher

LGTM

tlrx · 2017-02-15T12:54:09Z

Thanks @cbuescher @javanna

tlrx added :Java High Level REST Client >enhancement v6.0.0-alpha1 labels Feb 8, 2017

tlrx requested review from cbuescher and javanna February 8, 2017 09:25

javanna reviewed Feb 8, 2017

View reviewed changes

tlrx force-pushed the add-index-request-to-high-level-rest-client branch from e227619 to 3f8dc3c Compare February 10, 2017 11:13

cbuescher reviewed Feb 10, 2017

View reviewed changes

javanna approved these changes Feb 10, 2017

View reviewed changes

tlrx added 5 commits February 13, 2017 12:17

Add Index API to High Level Rest Client

7bcbff5

This commit adds support for the Index API in the High Level Rest Client.

Fix RequestTests

5e4622d

Revert changes in WriteRequest as requested by Luca

dfc5bf2

Update after reviews

56f5b9d

Use RefreshPolicy.getValue()

269e377

tlrx force-pushed the add-index-request-to-high-level-rest-client branch from 3f8dc3c to 269e377 Compare February 13, 2017 11:29

cbuescher approved these changes Feb 14, 2017

View reviewed changes

tlrx merged commit 86993ad into elastic:master Feb 15, 2017

tlrx deleted the add-index-request-to-high-level-rest-client branch February 15, 2017 12:54

javanna mentioned this pull request Jun 6, 2017

Backport high level REST client #25086

Merged

javanna mentioned this pull request Nov 1, 2017

Java high-level REST client completeness #27205

Closed

80 tasks

Conversation

tlrx commented Feb 8, 2017

Uh oh!

javanna Feb 8, 2017 • 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!

tlrx commented Feb 10, 2017

Uh oh!

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

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!

tlrx commented Feb 13, 2017

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

tlrx commented Feb 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

javanna Feb 8, 2017 •

edited

Loading