Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to disable chunked transfer-encoding #3864

Merged
merged 12 commits into from
Jul 13, 2022

Conversation

dblock
Copy link
Member

@dblock dblock commented Jul 12, 2022

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
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

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

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

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.

@dblock dblock marked this pull request as ready for review July 12, 2022 19:03
@dblock dblock requested review from a team and reta as code owners July 12, 2022 19:03
entity = new ContentCompressingEntity(entity);
entity = new ContentCompressingEntity(entity, chunkedTransferEncodingEnabled);
} else {
entity = new ContentHttpEntity(entity, chunkedTransferEncodingEnabled);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta by default entity.isChunked returns false

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@reta reta Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@reta reta Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
@dblock dblock requested a review from reta July 13, 2022 14:52
Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
dblock added 2 commits July 13, 2022 12:09
Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
@dblock
Copy link
Member Author

dblock commented Jul 13, 2022

@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
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dblock dblock merged commit f7894f7 into opensearch-project:main Jul 13, 2022
@dblock dblock deleted the jiten1551-main branch July 13, 2022 17:43
@dblock dblock added backport 1.x backport 2.x Backport to 2.x branch labels Jul 13, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 13, 2022
* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* adding chunk support for non-compressed request

Signed-off-by: Jitendra Kumar <[email protected]>

* Diable chunked transfer encoding in integration tests.

Signed-off-by: dblock <[email protected]>

* Replace chunkedTransferEncodingEnabled with chunkedEnabled.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Make chunkedEnabled optional.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove Optional<Boolean> constructor.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove optionals from constructors.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use Optional.empty()

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use a mode idiomatic syntax in isChunked.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

Co-authored-by: Jitendra Kumar <[email protected]>
(cherry picked from commit f7894f7)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 13, 2022
* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* adding chunk support for non-compressed request

Signed-off-by: Jitendra Kumar <[email protected]>

* Diable chunked transfer encoding in integration tests.

Signed-off-by: dblock <[email protected]>

* Replace chunkedTransferEncodingEnabled with chunkedEnabled.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Make chunkedEnabled optional.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove Optional<Boolean> constructor.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove optionals from constructors.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use Optional.empty()

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use a mode idiomatic syntax in isChunked.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

Co-authored-by: Jitendra Kumar <[email protected]>
(cherry picked from commit f7894f7)
reta pushed a commit that referenced this pull request Jul 13, 2022
* Add option to disable chunked transfer-encoding (#3864)

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* adding chunk support for non-compressed request

Signed-off-by: Jitendra Kumar <[email protected]>

* Diable chunked transfer encoding in integration tests.

Signed-off-by: dblock <[email protected]>

* Replace chunkedTransferEncodingEnabled with chunkedEnabled.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Make chunkedEnabled optional.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove Optional<Boolean> constructor.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove optionals from constructors.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use Optional.empty()

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use a mode idiomatic syntax in isChunked.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

Co-authored-by: Jitendra Kumar <[email protected]>
(cherry picked from commit f7894f7)

* Use JDK8-compatible implementation.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
dblock added a commit that referenced this pull request Jul 13, 2022
* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* fix for bug: #3640

Signed-off-by: Jitendra Kumar <[email protected]>

* adding chunk support for non-compressed request

Signed-off-by: Jitendra Kumar <[email protected]>

* Diable chunked transfer encoding in integration tests.

Signed-off-by: dblock <[email protected]>

* Replace chunkedTransferEncodingEnabled with chunkedEnabled.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Make chunkedEnabled optional.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove Optional<Boolean> constructor.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Remove optionals from constructors.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use Optional.empty()

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

* Use a mode idiomatic syntax in isChunked.

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

Co-authored-by: Jitendra Kumar <[email protected]>
(cherry picked from commit f7894f7)

Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.x backport 2.x Backport to 2.x branch
Projects
None yet
4 participants