Add option to disable chunked transfer-encoding #3864

dblock · 2022-07-12T17:10:42Z

Description

The goal is to give users an option to disable chunked transfer encoding for compressed streams which force chunked transfer encoding in the default implementation, which doesn't work for Sigv4.

Issues Resolved

Continuing #3665.

Closes #3640.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2022-07-12T17:12:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/452/
CommitID: e8b03c8ecd1cf0ba33e2c4e7b50b5663ebada28f

github-actions · 2022-07-12T17:14:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/453/
CommitID: e2a4fe6a2bf226d246e9d9a2231e740c5209d888

github-actions · 2022-07-12T17:17:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/454/
CommitID: 2a88ab8f6949f0c2465b8f5e080876bfea7d90db

github-actions · 2022-07-12T17:19:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/455/
CommitID: ca91501f45f4eb9ff10d56f9ac06fe995e7399f6

Signed-off-by: Jitendra Kumar <jkjitend@amazon.com>

github-actions · 2022-07-12T17:24:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/456/
CommitID: b12274e0b81e32c8ccf5a3cb766caede114ee18c

github-actions · 2022-07-12T17:25:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/457/
CommitID: 9074b6594f31522da5e894b9da698416b9f89b35

github-actions · 2022-07-12T17:30:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/458/
CommitID: 0d6158d0ea896327a0da3111af72112e937c7241

Signed-off-by: dblock <dblock@dblock.org>

github-actions · 2022-07-12T17:58:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/459/
CommitID: dc23a93

codecov-commenter · 2022-07-12T18:00:41Z

Codecov Report

Merging #3864 (a0d5295) into main (23004bd) will decrease coverage by 0.58%.
The diff coverage is 82.14%.

❗ Current head a0d5295 differs from pull request most recent head cc0f83f. Consider uploading reports for the commit cc0f83f to get more accurate results

@@             Coverage Diff              @@
##               main    #3864      +/-   ##
============================================
- Coverage     71.10%   70.51%   -0.59%     
+ Complexity    57060    56689     -371     
============================================
  Files          4557     4557              
  Lines        272681   272726      +45     
  Branches      40038    40046       +8     
============================================
- Hits         193884   192322    -1562     
- Misses        62773    64194    +1421     
- Partials      16024    16210     +186

Impacted Files	Coverage Δ
...rc/main/java/org/opensearch/client/RestClient.java	`89.03% <79.16%> (-2.28%)`	⬇️
.../java/org/opensearch/client/RestClientBuilder.java	`87.01% <100.00%> (+1.09%)`	⬆️
server/src/main/java/org/opensearch/Version.java	`79.53% <100.00%> (+0.09%)`	⬆️
...java/org/opensearch/client/indices/DataStream.java	`0.00% <0.00%> (-76.09%)`	⬇️
...n/indices/forcemerge/ForceMergeRequestBuilder.java	`0.00% <0.00%> (-75.00%)`	⬇️
...adonly/AddIndexBlockClusterStateUpdateRequest.java	`0.00% <0.00%> (-75.00%)`	⬇️
.../opensearch/client/indices/CloseIndexResponse.java	`17.50% <0.00%> (-73.75%)`	⬇️
...readonly/TransportVerifyShardIndexBlockAction.java	`9.75% <0.00%> (-58.54%)`	⬇️
.../java/org/opensearch/node/NodeClosedException.java	`50.00% <0.00%> (-50.00%)`	⬇️
...a/org/opensearch/tasks/TaskCancelledException.java	`50.00% <0.00%> (-50.00%)`	⬇️
... and 481 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1945119...cc0f83f. Read the comment docs.

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

reta · 2022-07-12T19:14:26Z

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

-                    entity = new ContentCompressingEntity(entity);
+                    entity = new ContentCompressingEntity(entity, chunkedTransferEncodingEnabled);
+                } else {
+                    entity = new ContentHttpEntity(entity, chunkedTransferEncodingEnabled);


We probably could do if (entity.isChunked() != chunkedTransferEncodingEnabled) { .. } and do wrapping, otherwise we should be good to go as-is , right?

@reta by default entity.isChunked returns false

@reta by default entity.isChunked returns false

@jiten1551 sorry, not sure I get it, HttpEntity is an interface, we deal with some of its implementation, where is the default you are referring to?

see https://github.com/opensearch-project/OpenSearch/blob/main/client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java#L755

here NByteArrayEntity is created for all request which extends AbstractHttpEntity which has default fault value of is chunked false as we're not setting it anywhere
https://www.javadoc.io/static/org.apache.httpcomponents/httpcore-nio/4.4.10/org/apache/http/nio/entity/NByteArrayEntity.html

We should not rely on any implementation details here, we deal with interface. Anyway, the question is: why we need to wrap the entity into ContentHttpEntity if chunking settings are not changing (true or false, does not really matter).

@jiten1551 I understand what this flag means:

if entity.isChunked() == false and chunkedTransferEncodingEnabled == false, why we need to wrap?

if entity.isChunked() == true and chunkedTransferEncodingEnabled == true, why we need to wrap?

in those cases we don't need to wrap but the thing is for non-compressed request entity.isChunked is always false
we're trying to give that support of chunking for non-compressed based on chunkedTransferEncodingEnabled.

in those cases we don't need to wrap

that's what I am suggesting to check here, nothing else (whatever assumptions may be deducted from implementation today may change in the future).

I changed the code to use an Optional and fallback to the entity.isChunked. It's a bit messy, lmk what you think?

I like it this way better (then dealing with nulls fe)

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

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

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

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

github-actions · 2022-07-13T15:15:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/518/
CommitID: 0cb85a4

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

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

dblock · 2022-07-13T15:27:37Z

~~@jiten1551 let us know when you've tested this change for your scenario and we will merge?~~

I was able to verify this.

github-actions · 2022-07-13T15:29:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/520/
CommitID: 718ce21

github-actions · 2022-07-13T15:35:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/521/
CommitID: a0d5295

github-actions · 2022-07-13T15:37:40Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/522/
CommitID: edb7d55

github-actions · 2022-07-13T15:50:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/524/
CommitID: cc0f83f

* fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * adding chunk support for non-compressed request Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * Diable chunked transfer encoding in integration tests. Signed-off-by: dblock <dblock@dblock.org> * Replace chunkedTransferEncodingEnabled with chunkedEnabled. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Make chunkedEnabled optional. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Remove Optional<Boolean> constructor. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Remove optionals from constructors. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Use Optional.empty() Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Use a mode idiomatic syntax in isChunked. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> Co-authored-by: Jitendra Kumar <jkjitend@amazon.com> (cherry picked from commit f7894f7)

* Add option to disable chunked transfer-encoding (#3864) * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * adding chunk support for non-compressed request Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * Diable chunked transfer encoding in integration tests. Signed-off-by: dblock <dblock@dblock.org> * Replace chunkedTransferEncodingEnabled with chunkedEnabled. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Make chunkedEnabled optional. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Remove Optional<Boolean> constructor. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Remove optionals from constructors. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Use Optional.empty() Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Use a mode idiomatic syntax in isChunked. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> Co-authored-by: Jitendra Kumar <jkjitend@amazon.com> (cherry picked from commit f7894f7) * Use JDK8-compatible implementation. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

* fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * fix for bug: #3640 Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * adding chunk support for non-compressed request Signed-off-by: Jitendra Kumar <jkjitend@amazon.com> * Diable chunked transfer encoding in integration tests. Signed-off-by: dblock <dblock@dblock.org> * Replace chunkedTransferEncodingEnabled with chunkedEnabled. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Make chunkedEnabled optional. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Remove Optional<Boolean> constructor. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Remove optionals from constructors. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Use Optional.empty() Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> * Use a mode idiomatic syntax in isChunked. Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com> Co-authored-by: Jitendra Kumar <jkjitend@amazon.com> (cherry picked from commit f7894f7) Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

dblock force-pushed the jiten1551-main branch from e8b03c8 to e2a4fe6 Compare July 12, 2022 17:12

dblock force-pushed the jiten1551-main branch from e2a4fe6 to 2a88ab8 Compare July 12, 2022 17:13

dblock force-pushed the jiten1551-main branch from 2a88ab8 to ca91501 Compare July 12, 2022 17:15

dblock force-pushed the jiten1551-main branch 2 times, most recently from b12274e to 9074b65 Compare July 12, 2022 17:18

dblock force-pushed the jiten1551-main branch from 9074b65 to 0d6158d Compare July 12, 2022 17:21

kumjiten added 5 commits July 12, 2022 17:23

fix for bug: opensearch-project#3640

7d7ad33

Signed-off-by: Jitendra Kumar <jkjitend@amazon.com>

fix for bug: opensearch-project#3640

a96873f

Signed-off-by: Jitendra Kumar <jkjitend@amazon.com>

fix for bug: opensearch-project#3640

213184c

Signed-off-by: Jitendra Kumar <jkjitend@amazon.com>

fix for bug: opensearch-project#3640

2b660cc

Signed-off-by: Jitendra Kumar <jkjitend@amazon.com>

adding chunk support for non-compressed request

d23b74b

Signed-off-by: Jitendra Kumar <jkjitend@amazon.com>

Diable chunked transfer encoding in integration tests.

dc23a93

Signed-off-by: dblock <dblock@dblock.org>

dblock force-pushed the jiten1551-main branch from 0d6158d to dc23a93 Compare July 12, 2022 17:34

dblock mentioned this pull request Jul 12, 2022

fix for bug: https://github.com/opensearch-project/OpenSearch/issues/… #3665

Closed

5 tasks

dblock marked this pull request as ready for review July 12, 2022 19:03

dblock requested review from a team and reta as code owners July 12, 2022 19:03

reta reviewed Jul 12, 2022

View reviewed changes

client/rest/src/main/java/org/opensearch/client/RestClient.java Outdated Show resolved Hide resolved

reta reviewed Jul 12, 2022

View reviewed changes

client/rest/src/main/java/org/opensearch/client/RestClient.java Outdated Show resolved Hide resolved

Replace chunkedTransferEncodingEnabled with chunkedEnabled.

5838aed

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

Make chunkedEnabled optional.

0cb85a4

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

dblock requested a review from reta July 13, 2022 14:52

Remove Optional<Boolean> constructor.

718ce21

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

reta reviewed Jul 13, 2022

View reviewed changes

client/rest/src/main/java/org/opensearch/client/RestClient.java Outdated Show resolved Hide resolved

dblock added 2 commits July 13, 2022 12:09

Remove optionals from constructors.

a0d5295

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

Use Optional.empty()

edb7d55

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

reta approved these changes Jul 13, 2022

View reviewed changes

reta reviewed Jul 13, 2022

View reviewed changes

client/rest/src/main/java/org/opensearch/client/RestClient.java Outdated Show resolved Hide resolved

Use a mode idiomatic syntax in isChunked.

cc0f83f

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

dblock merged commit f7894f7 into opensearch-project:main Jul 13, 2022

dblock deleted the jiten1551-main branch July 13, 2022 17:43

dblock added backport 1.x backport 2.x Backport to 2.x branch labels Jul 13, 2022

opensearch-trigger-bot bot mentioned this pull request Jul 13, 2022

[Backport 1.x] Add option to disable chunked transfer-encoding #3884

Merged

opensearch-trigger-bot bot mentioned this pull request Jul 13, 2022

[Backport 2.x] Add option to disable chunked transfer-encoding #3885

Merged

dblock mentioned this pull request Jul 14, 2022

Add support for signing chunked requests acm19/aws-request-signing-apache-interceptor#20

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to disable chunked transfer-encoding #3864

Add option to disable chunked transfer-encoding #3864

dblock commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

codecov-commenter commented Jul 12, 2022 •

edited

Loading

reta Jul 12, 2022

kumjiten Jul 13, 2022

reta Jul 13, 2022

kumjiten Jul 13, 2022

reta Jul 13, 2022 •

edited

Loading

reta Jul 13, 2022

kumjiten Jul 13, 2022

reta Jul 13, 2022

dblock Jul 13, 2022

reta Jul 13, 2022 •

edited

Loading

github-actions bot commented Jul 13, 2022

dblock commented Jul 13, 2022 •

edited

Loading

github-actions bot commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

Add option to disable chunked transfer-encoding #3864

Add option to disable chunked transfer-encoding #3864

Conversation

dblock commented Jul 12, 2022

Description

Issues Resolved

Check List

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 12, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jul 12, 2022 • edited Loading

Codecov Report

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

reta Jul 13, 2022 • edited Loading

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

reta Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2022

Gradle Check (Jenkins) Run Completed with:

dblock commented Jul 13, 2022 • edited Loading

github-actions bot commented Jul 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 13, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jul 12, 2022 •

edited

Loading

reta Jul 13, 2022 •

edited

Loading

reta Jul 13, 2022 •

edited

Loading

dblock commented Jul 13, 2022 •

edited

Loading