Sync streaming compression by davidh44 · Pull Request #4135 · aws/aws-sdk-java-v2

davidh44 · 2023-06-26T20:53:55Z

Motivation and Context

Implementing sync streaming compression

The ContentStreamProvider of the original RequestBody will be wrapped by CompressionContentStreamProvider, which will wrap the underlying InputStream with AwsCompressionInputStream. The AwsCompressionInputStream overrides read(byte[] b, int off, int len) to return compressed chunks.

Modifications

Added new class AwsChunkedEncodingInputStream to compress request in chunks.
Added overloaded compress() methods to Compressor and GzipCompressor to compress byte[] and ByteBuffer.
Renamed DecodedStreamBuffer to UnderlyingStreamBuffer for more general use

Testing

added tests

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

joviegas · 2023-06-26T20:59:09Z

Could you please update Motivation and Context specs/why/ what are the benefits of Chunked encoding for gzip ?

davidh44 · 2023-06-26T21:25:40Z

Could you please update Motivation and Context specs/why/ what are the benefits of Chunked encoding for gzip ?

Updated to add description

joviegas

Please add test cases to improve the coverage to 80% plus.https://sonarcloud.io/component_measures?id=aws_aws-sdk-java-v2&pullRequest=4135&metric=new_coverage&view=list

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/compression/GzipCompressor.java

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

...dk-core/src/main/java/software/amazon/awssdk/core/internal/io/AwsCompressionInputStream.java

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/Compressor.java

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/compression/GzipCompressor.java

millems · 2023-06-27T22:14:56Z

...ain/java/software/amazon/awssdk/core/internal/interceptor/RequestCompressionInterceptor.java

+        CompressionContentStreamProvider streamProvider =
+            new CompressionContentStreamProvider(requestBody.contentStreamProvider(), compressor);
+        return Optional.of(RequestBody.fromContentProvider(streamProvider, requestBody.contentType()));


It looks like this enables chunked encoding? Are we missing the Transfer-Encoding: chunked header? Are we making sure that there is no Content-Length header?

adding Transfer-Encoding : chunked header
Modeling tools should throw an error for operations with both streaming and requiresLength, will remove the Content-Length header just in case

…treaming-compression

joviegas · 2023-06-30T00:02:33Z

...ain/java/software/amazon/awssdk/core/internal/http/pipeline/stages/CompressRequestStage.java

-            input = updateContentEncodingHeader(input, compressor);
-            return updateContentLengthHeader(input);
+        // non-streaming OR transfer-encoding chunked
+        if (!isStreaming(context) || isTransferEncodingChunked(input)) {


In which case we use isTransferEncodingChunked for non streaming ? I was assuming that transferEncodingChunked is always for streaming.
How is isTransferEncodingChunked() set on the input ? is it via user or on the model itself

Correct, TE:chunked is only for streaming. This is checking for the 2 separate cases, not the combined case.

The specification states that when the request is already TE : chunked , we should compress the entire stream first, instead of compressing in chunks.

It is set in the marshallers: AbstractStreamingRequestMarshaller and JsonProtocolMarshaller

...ain/java/software/amazon/awssdk/core/internal/http/pipeline/stages/CompressRequestStage.java

...dk-core/src/main/java/software/amazon/awssdk/core/internal/io/AwsCompressionInputStream.java

...dwatch/src/it/java/software/amazon/awssdk/services/cloudwatch/CloudWatchIntegrationTest.java

joviegas · 2023-07-06T17:59:14Z

https://sonarcloud.io/component_measures?id=aws_aws-sdk-java-v2&pullRequest=4135&metric=new_coverage&view=list
Is showing only 23% can we increase this to 80 and update the test cases to cover

davidh44 · 2023-07-06T18:16:22Z

https://sonarcloud.io/component_measures?id=aws_aws-sdk-java-v2&pullRequest=4135&metric=new_coverage&view=list Is showing only 23% can we increase this to 80 and update the test cases to cover

This was run before the latest commits. Looks like a Github issue, as the commits were missing from the PR (had to change base branch to fix). Retried the checks through CodeBuild, running

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/CompressionType.java

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/AmazonAsyncHttpClient.java

...ain/java/software/amazon/awssdk/core/internal/http/pipeline/stages/CompressRequestStage.java

joviegas · 2023-07-06T18:24:20Z

...ain/java/software/amazon/awssdk/core/internal/http/pipeline/stages/CompressRequestStage.java

-
-        long contentLength = Long.parseLong(input.firstMatchingHeader("Content-Length").orElse("0"));
-        return contentLength >= minimumCompressionThreshold;
+        int requestSize = SdkBytes.fromInputStream(input.contentStreamProvider().newStream()).asByteArray().length;


We are reading the entire stream here to get the request size,
We should avoid intermediate reading of the streams this can cause performance impact , can we discuss this with team ?

the content length header isn't set yet for nonstreaming operations. not sure how we would get the content length if not by reading the stream, can discuss with team

updated method to check for content length header first, and if not present, then read the stream to determine length

...-core/src/test/java/software/amazon/awssdk/core/internal/compression/GzipCompressorTest.java

...rated-classes-test/src/test/java/software/amazon/awssdk/services/RequestCompressionTest.java

joviegas · 2023-07-09T01:15:01Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/compression/CompressionType.java

+    }
+
+    @Override
+    public int hashCode() {


We can improve the Coverage of newly added classes by performing EqualsVerifier test , this will make sure equals and hashcode are tested
(generic comment for newly added classes as part of this feature)

import nl.jqno.equalsverifier.EqualsVerifier;

added tests for CompressionType and RequestCompressionConfiguration

sonarqubecloud · 2023-07-12T18:43:26Z

SonarCloud Quality Gate failed.

13 Bugs
0 Vulnerabilities
0 Security Hotspots
45 Code Smells

70.3% Coverage
4.3% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

…4bbc94a63 Pull request: release <- staging/06b7e1ba-f147-418e-94ce-8504bbc94a63

davidh44 added 3 commits June 26, 2023 13:48

Rename DecodedStreamBuffer to UnderlyingStreamBuffer

157fc10

Update Compressor

29380ab

Sync streaming compression

228a035

davidh44 requested a review from a team as a code owner June 26, 2023 20:53

Fix Checkstyle issues

280bd38

joviegas reviewed Jun 27, 2023

View reviewed changes

millems reviewed Jun 27, 2023

View reviewed changes

davidh44 added 9 commits June 28, 2023 14:10

Refactor to common class AwsChunkedInputStream

e2d6951

Refactor Compressor

be06647

Merge from feature/master/request-compression

04b5e7c

Refactor CompressionType

5b5641c

sync compression

7e07424

Close GZIPOutputStream

ba7e602

Add compress stage to async http client

5b37111

Add compressed PutMetricData integ test

cd241a6

Merge branch 'feature/master/request-compression' into hdavidh/sync-s…

0f4fd59

…treaming-compression

joviegas reviewed Jun 30, 2023

View reviewed changes

davidh44 added 2 commits July 4, 2023 22:10

Refactoring

5268c0b

Add tests

d8aa9a5

davidh44 changed the base branch from feature/master/request-compression to master July 6, 2023 16:15

davidh44 changed the base branch from master to feature/master/request-compression July 6, 2023 16:15

davidh44 closed this Jul 6, 2023

davidh44 reopened this Jul 6, 2023

joviegas reviewed Jul 6, 2023

View reviewed changes

davidh44 added 2 commits July 7, 2023 14:23

Add equals and hashCode

37d962e

Add TODO

aab6f62

Add tests with retry

4b52a88

joviegas reviewed Jul 9, 2023

View reviewed changes

davidh44 added 6 commits July 10, 2023 17:23

Update content length retrieval

d9e28d9

CompressionType test

86ff75a

RequestCompressionConfiguration test

24d69db

Fix Spotbugs error

9dadea3

Rename CompressionType to CompressorType

95e0383

Fix import ordering

4d1e924

davidh44 added 4 commits July 14, 2023 16:58

Remove chunk headers and trailers

ebc0c35

Update tests

1d9bb6f

Refactoring

36dd8ef

Only compress streaming operations that are chunked

518e4c0

davidh44 mentioned this pull request Jul 21, 2023

Request compression sync streaming #4222

Merged

12 tasks

davidh44 closed this Jul 21, 2023

davidh44 deleted the hdavidh/sync-streaming-compression branch August 29, 2023 23:44

aws-sdk-java-automation added a commit that referenced this pull request Jul 18, 2025

Merge pull request #4135 from aws/staging/06b7e1ba-f147-418e-94ce-850…

bc47d8d

…4bbc94a63 Pull request: release <- staging/06b7e1ba-f147-418e-94ce-8504bbc94a63

Conversation

davidh44 commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

joviegas commented Jun 26, 2023

Uh oh!

davidh44 commented Jun 26, 2023

Uh oh!

joviegas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joviegas commented Jul 6, 2023

Uh oh!

davidh44 commented Jul 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

joviegas Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidh44 commented Jun 26, 2023 •

edited

Loading

joviegas left a comment •

edited

Loading

joviegas Jul 9, 2023 •

edited

Loading