REST Client: Add Request object flavored methods #29623

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

 import java.util.StringJoiner;

-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.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 #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/qa/audit-tests`, `x-pack/qa/ml-disabled`, and `x-pack/qa/multi-node` 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/smoke-test-monitoring-with-watcher`, `x-pack/qa/smoke-test-watcher`, and `x-pack/qa/smoke-test-watcher-with-security` projects 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/qa/smoke-test-monitoring-with-watcher`, `x-pack/qa/smoke-test-watcher`, and `x-pack/qa/smoke-test-watcher-with-security` projects 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/qa/ml-basic-multi-node` 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/ml/qa/native-multi-node-tests`, `x-pack/plugin/ml/qa/single-node-tests` 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/saml-idp-tests` and `x-pack/qa/security-setup-password-tests` 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 `client` and `distribution` projects 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/ml/qa/native-multi-node-tests`, `x-pack/plugin/ml/qa/single-node-tests` projects 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 `client` and `distribution` projects 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/qa/saml-idp-tests` and `x-pack/qa/security-setup-password-tests` 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. In a long series of PRs I've changed all of the old style requests that I could find with `grep`. In this PR I change all requests that I could find by *removing* the deprecated methods. Since this is a non-trivial change I do not include actually removing the deprecated requests. I'll do that in a follow up. But this should be the last set of usage removals before the actual deprecated method removal. Yay!

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/ml/qa/native-multi-node-tests`, `x-pack/plugin/ml/qa/single-node-tests` projects 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/qa/saml-idp-tests` and `x-pack/qa/security-setup-password-tests` projects 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 `client` and `distribution` projects 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. In a long series of PRs I've changed all of the old style requests that I could find with `grep`. In this PR I change all requests that I could find by *removing* the deprecated methods. Since this is a non-trivial change I do not include actually removing the deprecated requests. I'll do that in a follow up. But this should be the last set of usage removals before the actual deprecated method removal. Yay!

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. In a long series of PRs I've changed all of the old style requests. This drops the deprecated methods and will be released with 7.0.

In #29623 we added `Request` object flavored requests to the low level REST client and in #30315 we deprecated the old `performRequest`s. In a long series of PRs I've changed all of the old style requests. This drops the deprecated methods and will be released with 7.0.

In #29623 we added `Request` object flavored requests to the low level REST client and in #30315 we deprecated the old `performRequest`s. In a long series of PRs I've changed all of the old style requests that I could find with `grep`. In this PR I change all requests that I could find by *removing* the deprecated methods from master. Note: I'm not actually removing all calls to the deprecated methods in ths commit. I'm just backporting the commit from master that removes all of the deprecated calls so future backports from master will be clean. There is *probably* still calls to the deprecated methods in the 6.x branch which is just fine because we'll only be dropping this methods from master.

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 Aug 21, 2018

Switch some watcher tests to new style Requests #33044

Merged

nik9000 mentioned this pull request Aug 23, 2018

Switch remaining ml tests to new style Requests #33107

Merged

nik9000 mentioned this pull request Aug 23, 2018

Switch remaining x-pack tests to new style Requests #33108

Merged

nik9000 mentioned this pull request Aug 23, 2018

Switch remaining tests to new style Requests #33109

Merged

nik9000 mentioned this pull request Aug 27, 2018

Switch remaining LLREST usage to new style Requests #33171

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST Client: Add Request object flavored methods #29623

REST Client: Add Request object flavored methods #29623

nik9000 commented Apr 19, 2018

elasticmachine commented Apr 19, 2018

javanna left a comment

javanna Apr 24, 2018

javanna Apr 24, 2018

javanna Apr 24, 2018

nik9000 Apr 26, 2018

javanna Apr 24, 2018

nik9000 Apr 26, 2018

javanna Apr 24, 2018

nik9000 Apr 26, 2018

javanna Apr 24, 2018

javanna Apr 24, 2018

nik9000 Apr 26, 2018

javanna Apr 30, 2018

javanna Apr 24, 2018

nik9000 Apr 26, 2018

javanna Apr 30, 2018

nik9000 Apr 26, 2018

nik9000 commented Apr 26, 2018

REST Client: Add Request object flavored methods #29623

REST Client: Add Request object flavored methods #29623

Conversation

nik9000 commented Apr 19, 2018

elasticmachine commented Apr 19, 2018

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

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

nik9000 commented Apr 26, 2018