REST Client: Add Request object flavored methods by nik9000 · Pull Request #29623 · elastic/elasticsearch

nik9000 · 2018-04-19T21:13:03Z

Adds two new methods to RestClient that take a Request object. These
methods will allows us to add more per-request customizable options
without creating more and more and more overloads of the performRequest
and performRequestAsync methods. These new methods look like:

Response performRequest(Request request)

and

void performRequestAsync(Request request, ResponseListener responseListener)

This change doesn't add any actual features but enables adding things like
per request timeouts and per request node selectors. This change does
rework the HighLevelRestClient and its tests to use these new Request
objects and it does update the docs.

This reverts commit 1cf984f.

The test looks like it is asserting stuff about the request but that duplicates what is in `HighLevelRequestsTests`.

elasticmachine · 2018-04-19T21:13:05Z

Pinging @elastic/es-core-infra

javanna

I left some comments but they are all minor. LGTM otherwise. I wonder if we should take out the deprecations and include them in a followup PR that can have the deprecation label and more visibility. Also, this will go 6.x too right?

javanna · 2018-04-24T10:58:03Z

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


-public final class Request {
-
+final class HighLevelRequests {


++ also on the visibility change

javanna · 2018-04-24T10:59:45Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

+import java.util.Objects;
+
+/**
+ * Request to Elasticsearch.


HTTP request?

javanna · 2018-04-24T11:00:59Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

+    private final String method;
+    private final String endpoint;
+
+    private Map<String, String> parameters = Collections.<String, String>emptyMap();


nit: shall we also make the parameters final? We could expose an add method if we want to rather than set. not too sure myself though about this, do what you prefer, just an idea.

I'll check on that!

javanna · 2018-04-24T11:06:33Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+        } catch (Exception e) {
+            responseListener.onFailure(e);
+            return;
+        }


shall we have a small private method that allows to build the request given all the different params and incorporates the catch as well?

I didn't do that because each method sets different stuff.

javanna · 2018-04-24T11:23:20Z

client/rest/src/test/java/org/elasticsearch/client/RequestTests.java

+        assertArrayEquals(headers, request.getHeaders());
+    }
+
+    // TODO equals and hashcode


++ I have been wondering if we could make add a test dep to our test-framework. I think the reason why we haven't done it before is the java7 requirement.

Yeah, the java 7 requirement is it I think.

javanna · 2018-04-24T11:25:57Z

docs/java-rest/low-level/usage.asciidoc

+`performRequest` or `performRequestAsync`. `performRequest` is synchronous and
+will block the calling thread and return the `Response` when the request is
+complete. `performRequestAsync` is asynchronous and accepts a `ResponseListener`
+argument that it will call with the request is complete or fails.


this last sentence doesn't compile

javanna · 2018-04-24T11:27:44Z

docs/java-rest/low-level/usage.asciidoc

-callback instance per request attempt. Controls how the response body gets
-streamed from a non-blocking HTTP connection on the client side. When not
-provided, the default implementation is used which buffers the whole response
-body in heap memory, up to 100 MB.


part of this explanation was useful I think, like the explanation of the default limit, which says why you may need to provide your own impl. Thoughts?

I've added the explanation back.

have you removed the link to the HttpAsyncResponseConsumer javadocs on purpose? You think it was not useful?

javanna · 2018-04-24T11:34:58Z

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

-import java.util.Objects;
 import java.util.StringJoiner;

-public final class Request {


one more thing, not too convinced about naming, given that what these methods do is convert from high-level requests to low-level requests. maybe rather Requests or RequestUtils? not a big deal though.

I'll rename it to RequestConverters. OK?

nik9000 · 2018-04-26T17:36:56Z

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

@@ -1370,143 +1343,115 @@ private static void assertToXContentBody(ToXContent expectedBody, HttpEntity act
        assertEquals(expectedBytes, new BytesArray(EntityUtils.toByteArray(actualEntity)));
    }

-    public void testParams() {


This behavior has all been migrated to the Request object and has a test in the low level REST client.

nik9000 · 2018-04-26T17:57:52Z

@javanna, I've worked through most of your comments. The one about using a final Map for the parameters I think helped me to clean up a whole bunch of stuff. It'd be awesome if you'd have another look to make sure you like where I took those.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack:core` project to use the new versions.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack/plugin/monitoring` project to use the new versions.

In #29623 we added `Request` object flavored requests to the low level REST client and in #30315 we deprecated the old `performRequest`s. This changes all calls in the `qa/full-cluster-restart` project to use the new versions. It also fixes a small bug in the test for explain on the `_all` field that was causing it to not properly invoke `_explain`.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack/plugin/security` project to use the new versions.

In #29623 we added `Request` object flavored requests to the low level REST client and in #30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack:core` project to use the new versions.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack:qa:full-cluster-restart` project to use the new versions.

In #29623 we added `Request` object flavored requests to the low level REST client and in #30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack:core` project to use the new versions.

In #29623 we added `Request` object flavored requests to the low level REST client and in #30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack/plugin/monitoring` project to use the new versions.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack/plugin` project to use the new versions.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack:qa:rolling-upgrade*` projects to use the new versions.

In elastic#29623 we added `Request` object flavored requests to the low level REST client and in elastic#30315 we deprecated the old `performRequest`s. This changes all calls in the `x-pack/qa/security-example-spi-extension` project to use the new versions.

nik9000 added 11 commits April 2, 2018 12:39

Low level client

bbb82fe

WIP

1cf984f

Merge branch 'master' into rest_requests

0de81af

Revert "WIP"

1098731

This reverts commit 1cf984f.

Merge branch 'master' into rest_requests

b5d04b6

Update

fa4f204

Remove deprecateds

16191b3

WIP

7938724

Merge branch 'master' into rest_requests

9067cf8

Rework and drop some assertions

9d4b4e3

The test looks like it is asserting stuff about the request but that duplicates what is in `HighLevelRequestsTests`.

Docs

e0788a0

nik9000 added >enhancement review :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v7.0.0 labels Apr 19, 2018

nik9000 requested a review from javanna April 19, 2018 21:13

nik9000 changed the title ~~REST Client: Add request object flavored methods~~ REST Client: Add Request object flavored methods Apr 19, 2018

javanna approved these changes Apr 24, 2018

View reviewed changes

javanna reviewed Apr 24, 2018

View reviewed changes

nik9000 added 4 commits April 26, 2018 11:25

Merge branch 'master' into rest_requests

38be87d

Fixup words

28bb2a6

Rename to RequestConverters

a75ffd2

Make parameters final and make clients add to it

3987664

nik9000 commented Apr 26, 2018

View reviewed changes

nik9000 added 4 commits April 26, 2018 13:37

Cleanup

7ba9517

Merge branch 'master' into rest_requests

2990265

Update merged code

8f8bba0

Ooops

72bcab6

nik9000 mentioned this pull request Jul 20, 2018

Switch x-pack:plugin:core to new style Requests #32252

Merged

nik9000 mentioned this pull request Jul 20, 2018

Switch monitoring to new style Requests #32255

Merged

nik9000 mentioned this pull request Jul 23, 2018

Switch security to new style Requests #32290

Merged

nik9000 mentioned this pull request Jul 23, 2018

Switch x-pack full restart to new style Requests #32294

Merged

nik9000 mentioned this pull request Jul 24, 2018

Switch x-pack/plugin to new style Requests #32327

Merged

nik9000 mentioned this pull request Jul 24, 2018

Switch x-pack rolling restart tests to new style Requests #32339

Merged

This was referenced Jul 24, 2018

Switch security spi example to new style Requests #32341

Merged

Switch ml basic tests to new style Requests #32483

Merged

Switch some x-pack tests to new style Requests #32500

Merged

nik9000 mentioned this pull request Aug 29, 2018

LLREST: Drop deprecated methods #33223

Merged

pvliss mentioned this pull request Jan 15, 2019

Liquibase - fix floatType for mariadb jhipster/generator-jhipster#9063

Merged

4 tasks

Conversation

nik9000 commented Apr 19, 2018

Uh oh!

elasticmachine commented Apr 19, 2018

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!

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!

nik9000 commented Apr 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants