-
Notifications
You must be signed in to change notification settings - Fork 20
Add support for request compression #968
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
base: main
Are you sure you want to change the base?
Conversation
client/client-http/src/main/java/software/amazon/smithy/java/client/http/compression/Gzip.java
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/java/client/http/compression/CompressionAlgorithm.java
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/java/client/http/plugins/RequestCompressionPlugin.java
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/java/client/http/compression/CompressionAlgorithm.java
Outdated
Show resolved
Hide resolved
io/src/main/java/software/amazon/smithy/java/io/datastream/GzipInputStream.java
Outdated
Show resolved
Hide resolved
io/src/main/java/software/amazon/smithy/java/io/datastream/GzipInputStream.java
Outdated
Show resolved
Hide resolved
client/client-http/src/main/java/software/amazon/smithy/java/client/http/HttpContext.java
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/java/client/http/plugins/RequestCompressionPlugin.java
Outdated
Show resolved
Hide resolved
client/client-http/src/main/java/software/amazon/smithy/java/client/http/compression/Gzip.java
Outdated
Show resolved
Hide resolved
client/client-http/src/main/java/software/amazon/smithy/java/client/http/compression/Gzip.java
Show resolved
Hide resolved
io/src/main/java/software/amazon/smithy/java/io/datastream/GzipInputStream.java
Outdated
Show resolved
Hide resolved
|
More in general, can you please use consistently |
| import java.io.InputStream; | ||
| import java.util.zip.GZIPOutputStream; | ||
|
|
||
| final class GzipCompressingInputStream extends InputStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation underneath still reads the whole input stream into memory to do the compression.
| if (available <= 0) | ||
| return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use brackets.
| try (GZIPOutputStream gzip = new GZIPOutputStream(buffer)) { | ||
| source.transferTo(gzip); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads all the contents from source and transfers them to the gzip output stream.
| CompressionAlgorithm.supportedAlgorithms(); | ||
|
|
||
| @Override | ||
| public <RequestT> RequestT modifyBeforeTransmit(RequestHook<?, ?, RequestT> hook) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use here modifyBeforeRetryLoop instead. We want to compress only once instead of for each retry.
Also, we need to make sure that compression and streaming plays well with retries.
This PR added a new plugin to process request compression if
@requestCompressiontrait is applied. With this feature, those protocol tests regarding content-encoding can pass now.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.