Add Index API to High Level Rest Client #23040

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

        /**
         * Leave this request open until a refresh has made the contents of this request visible to search. This refresh policy is
         * compatible with high indexing and search throughput but it causes the request to wait to reply until a refresh occurs.
         */
-        WAIT_UNTIL;
+        WAIT_UNTIL((byte) 2, "wait_for");


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

+        }
+
+        Params withRefreshPolicy(WriteRequest.RefreshPolicy refreshPolicy) {
+            if (refreshPolicy != WriteRequest.RefreshPolicy.NONE) {


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

+        return parameters.getParams();
+    }
+
+    private static String getEndpoint(GetRequest getRequest) {


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

+        return pathJoiner.add(getRequest.index()).add(getRequest.type()).add(getRequest.id()).toString();
+    }
+
+    static Request index(IndexRequest indexRequest) {


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

+            }
+        }
+
+        final XContentType xContentType = randomFrom(XContentType.values());


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

+
+        Map<String, String> expectedParams = new HashMap<>();
+        long version = randomFrom(Versions.MATCH_DELETED, Versions.NOT_FOUND, randomNonNegativeLong());
+        indexRequest.version(version);


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

+                builder.field("field_" + i, i);
+            }
+            builder.endObject();
+            indexRequest.source(builder);


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

+    private static class Endpoint {
+        private final List<String> parts = new ArrayList<>();
+        private final String delimiter = "/";
+        private final String prefix = "/";


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

+        Endpoint endpoint = Endpoint.builder();
+        endpoint.add(indexRequest.index());
+        endpoint.add(indexRequest.type());
+        endpoint.add(indexRequest.id());


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

+        private Endpoint() {
+        }
+
+        Endpoint withSuffix(String suffix) {


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

+        }
+
+        Params withRefreshPolicy(WriteRequest.RefreshPolicy refreshPolicy) {
+            if (refreshPolicy != WriteRequest.RefreshPolicy.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?

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

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

+            assertEquals(RestStatus.CONFLICT, exception.status());
+            assertEquals("Elasticsearch exception [type=version_conflict_engine_exception, reason=[type][with_create_op_type]: " +
+                         "version conflict, document already exists (current version [1])]", exception.getMessage());
+        }


very nice tests, thanks!

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

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

+    }
+
+    static Request index(IndexRequest indexRequest) {
+        String method = Strings.hasLength(indexRequest.id()) ? "PUT" : "POST";


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

+        return parameters.getParams();
+    }
+
+    private static String getEndpoint(GetRequest getRequest) {


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 javanna and cbuescher 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Index API to High Level Rest Client #23040

Add Index API to High Level Rest Client #23040

tlrx commented Feb 8, 2017

javanna Feb 8, 2017 •

edited

Loading

tlrx Feb 10, 2017

tlrx Feb 10, 2017

tlrx commented Feb 10, 2017

cbuescher left a comment

cbuescher Feb 10, 2017

cbuescher Feb 10, 2017

tlrx Feb 10, 2017

javanna Feb 10, 2017

tlrx Feb 13, 2017

cbuescher Feb 10, 2017

tlrx Feb 10, 2017

javanna Feb 10, 2017

cbuescher Feb 10, 2017

cbuescher Feb 10, 2017

tlrx Feb 13, 2017

cbuescher Feb 10, 2017

tlrx Feb 10, 2017

cbuescher Feb 10, 2017

javanna left a comment

javanna Feb 10, 2017

javanna Feb 10, 2017

tlrx Feb 13, 2017

tlrx Feb 13, 2017

javanna Feb 10, 2017

javanna Feb 10, 2017

tlrx Feb 13, 2017

javanna Feb 10, 2017

javanna Feb 10, 2017

javanna Feb 10, 2017

javanna Feb 10, 2017

tlrx commented Feb 13, 2017

cbuescher left a comment

tlrx commented Feb 15, 2017

Add Index API to High Level Rest Client #23040

Add Index API to High Level Rest Client #23040

Conversation

tlrx commented Feb 8, 2017

javanna Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Feb 10, 2017

cbuescher 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna 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

tlrx commented Feb 13, 2017

cbuescher left a comment

Choose a reason for hiding this comment

tlrx commented Feb 15, 2017

javanna Feb 8, 2017 •

edited

Loading