LLClient: Add String flavored setEntity by nik9000 · Pull Request #30447 · elastic/elasticsearch

nik9000 · 2018-05-07T23:39:07Z

Adds Request#setEntity(String) which short circuits the process of
sending a json string which is super common.

Adds `Request#setEntity(String)` which short circuits the process of sending a json string which is super common.

elasticmachine · 2018-05-07T23:39:08Z

Pinging @elastic/es-core-infra

nik9000 · 2018-05-07T23:41:47Z

@javanna this follows up on some comments you've made on my recent PRs to use the Request flavored methods in the http client.

I think this extra method would be pretty useful. I'm not really sold on a byte[] version of the method though because it feels like we're adding more and more flavors for less and less common things. So I figure I'd propose this change and see where it goes.

dadoonet

I like that change! 😉
I left 2 comments.

dadoonet · 2018-05-08T00:42:30Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

            entity.endObject();
-            BytesRef bytes = BytesReference.bytes(entity).toBytesRef();
-            request.setEntity(new ByteArrayEntity(bytes.bytes, bytes.offset, bytes.length, ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


Should we support as well the method for bytes?

Ha! Just saw your other comment.

on this one I am on the fence. It is odd that we have to go from bytes to string here. I would rather leave what you had, or add a method to set a byte array, even if we are the only ones using it.

dadoonet · 2018-05-08T00:43:32Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

                    .field("scroll_id", scroll)
                .endObject();
-            request.setEntity(new StringEntity(Strings.toString(entity), ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


Should we support the method for XContent as well?

low-level client doesn't depend on xcontent, we can't do that

javanna

left a few nits, LGTM otherwise. thanks @nik9000 !

javanna · 2018-05-08T09:53:12Z

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

+     * If you need a different content type then use
+     * {@link #setEntity(HttpEntity)}.
+     */
+    public void setEntity(String entity) {


nit: call it setJsonEntity?

javanna · 2018-05-08T09:58:03Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

                    .field("scroll_id", scroll)
                .endObject();
-            request.setEntity(new StringEntity(Strings.toString(entity), ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


low-level client doesn't depend on xcontent, we can't do that

javanna · 2018-05-08T09:58:53Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java

            entity.endObject();
-            BytesRef bytes = BytesReference.bytes(entity).toBytesRef();
-            request.setEntity(new ByteArrayEntity(bytes.bytes, bytes.offset, bytes.length, ContentType.APPLICATION_JSON));
+            request.setEntity(Strings.toString(entity));


on this one I am on the fence. It is odd that we have to go from bytes to string here. I would rather leave what you had, or add a method to set a byte array, even if we are the only ones using it.

javanna · 2018-05-08T09:59:27Z

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

+     * {@link #setEntity(HttpEntity)}.
+     */
+    public void setEntity(String entity) {
+        setEntity(entity == null ? null : new NStringEntity(entity, ContentType.APPLICATION_JSON));


can we add a small unit test for this method?

nik9000 · 2018-05-09T14:00:52Z

Yikes! I'm not sure how I let the failure through....

nik9000 · 2018-05-09T22:34:46Z

Thanks for having a look @javanna and @dadoonet! I've merged and am cherry-picking back to 6.x now.

Adds `Request#setJsonEntity(String)` which short circuits the process of sending a json string which is super common.

* master: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) add version compatibility from 6.4.0 after backport, see #30319 (#30390) Security: Simplify security index listeners (#30466) Add proper longitude validation in geo_polygon_query (#30497) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Solve Gradle deprecation warnings around shadowJar (#30483) SAML: Process only signed data (#30420) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163) Silence IndexUpgradeIT test failures. (#30430) Bump Gradle heap to 1792m (#30484) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) Test: remove hardcoded list of unconfigured ciphers (#30367) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) BulkProcessor to retry based on status code (#29329) Add GET Repository High Level REST API (#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (#30313) Stop forking groovyc (#30471) Avoid setting connection request timeout (#30384) Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) # Conflicts: # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

* 6.x: Upgrade to Lucene-7.4-snapshot-6705632810 (#30519) Remove Discovery.AckListener.onTimeout() (#30514) Build: move generated-resources to build (#30366) Reindex: Fold "with all deps" project into reindex (#30154) Isolate REST client single host tests (#30504) Remove BWC repository test (#30500) Build: Remove xpack specific run task (#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests LLClient: Add setJsonEntity (#30447) [docs] add warning for read-write indices in force merge documentation (#28869) Avoid deadlocks in cache (#30461) BulkProcessor to retry based on status code (#29329) Avoid setting connection request timeout (#30384) Test: remove hardcoded list of unconfigured ciphers (#30367) Add GET Repository High Level REST API (#30362) mute SplitIndexIT due to #30416 Docs: Test examples that recreate lang analyzers (#29535) add a comment explaining the need for RetryOnReplicaException on missing mappings Pass the task to broadcast actions (#29672) Stop forking groovyc (#30471) Add `coordinating_only` node selector (#30313) Fix accidental error in changelog Use date format in `date_range` mapping before fallback to default (#29310) Watcher: Increase HttpClient parallel sent requests (#30130) [Security][Tests] Azeri(Turkish) locale tripps opensaml dependency

* es/ccr: (78 commits) Upgrade to Lucene-7.4-snapshot-6705632810 (elastic#30519) add version compatibility from 6.4.0 after backport, see elastic#30319 (elastic#30390) Security: Simplify security index listeners (elastic#30466) Add proper longitude validation in geo_polygon_query (elastic#30497) Remove Discovery.AckListener.onTimeout() (elastic#30514) Build: move generated-resources to build (elastic#30366) Reindex: Fold "with all deps" project into reindex (elastic#30154) Isolate REST client single host tests (elastic#30504) Solve Gradle deprecation warnings around shadowJar (elastic#30483) SAML: Process only signed data (elastic#30420) Remove BWC repository test (elastic#30500) Build: Remove xpack specific run task (elastic#30487) AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests Enable soft-deletes in v6.4 LLClient: Add setJsonEntity (elastic#30447) [CCR] Read changes from Lucene instead of translog (elastic#30120) Expose CommonStatsFlags directly in IndicesStatsRequest. (elastic#30163) Silence IndexUpgradeIT test failures. (elastic#30430) Bump Gradle heap to 1792m (elastic#30484) [docs] add warning for read-write indices in force merge documentation (elastic#28869) ...

LLClient: Add String flavored setEntity

9e54247

Adds `Request#setEntity(String)` which short circuits the process of sending a json string which is super common.

nik9000 added >enhancement review :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v7.0.0 v6.4.0 labels May 7, 2018

nik9000 requested a review from javanna May 7, 2018 23:39

dadoonet reviewed May 8, 2018

View reviewed changes

javanna approved these changes May 8, 2018

View reviewed changes

nik9000 added 6 commits May 8, 2018 08:28

Rename

01e983c

Merge branch 'master' into rest_client_set_entity_2

84def91

Rename and unit test

04e9700

Changelog

2fe40c5

Merge branch 'master' into rest_client_set_entity_2

ea564a1

Merge branch 'master' into rest_client_set_entity_2

6122f94

nik9000 added 2 commits May 9, 2018 10:01

Merge branch 'master' into rest_client_set_entity_2

96ea9bc

Fix unit test

d946a6f

nik9000 merged commit b4502db into elastic:master May 9, 2018

nik9000 added a commit that referenced this pull request May 9, 2018

LLClient: Add setJsonEntity (#30447)

51a38cf

Adds `Request#setJsonEntity(String)` which short circuits the process of sending a json string which is super common.

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

Conversation

nik9000 commented May 7, 2018

Uh oh!

elasticmachine commented May 7, 2018

Uh oh!

nik9000 commented May 7, 2018

Uh oh!

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

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!

nik9000 commented May 9, 2018

Uh oh!

nik9000 commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants