-
Notifications
You must be signed in to change notification settings - Fork 966
feat: allow explicitly enabling/disabling of encodings in EncodingService
#6398
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
Conversation
EncodingService
EncodingServiceEncodingService
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6398 +/- ##
============================================
- Coverage 74.46% 74.11% -0.36%
- Complexity 22234 23005 +771
============================================
Files 1963 2062 +99
Lines 82437 86154 +3717
Branches 10764 11310 +546
============================================
+ Hits 61385 63851 +2466
- Misses 15918 16890 +972
- Partials 5134 5413 +279 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
core/src/main/java/com/linecorp/armeria/server/encoding/EncodingServiceBuilder.java
Outdated
Show resolved
Hide resolved
Just leave a general link to the RFC and expect that users will open the RFC side-by-side.
schiemon
left a comment
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.
If tests are not broken, we may remove it. |
ikhoon
left a comment
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.
Overall looks good
core/src/main/java/com/linecorp/armeria/common/encoding/StreamEncoderFactories.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/encoding/StreamEncoderFactory.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncoders.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncoders.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncoders.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncoders.java
Show resolved
Hide resolved
- adjust some style - transition from block to line comments
minwoox
left a comment
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.
Looks great, thanks!
core/src/main/java/com/linecorp/armeria/server/encoding/EncodingServiceBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncoders.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/encoding/HttpEncoders.java
Show resolved
Hide resolved
|
@ikhoon May I ask for another review to get a ✅ ? :) |
ikhoon
left a comment
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.
Thanks! 👍👍
Motivation
From #6390:
Modifications
Moved
StreamEncoderFactory,StreamEncoderFactories, andSnappyFramedOutputStreamfromcom.linecorp.armeria.internal.common.encodingtocom.linecorp.armeria.common.encodingto enable their use inEncoderServiceand related components.Modified
HttpEncoders.determineEncoderto accept a map ofStreamEncoderFactorys that defines the availableStreamEncoderFactoryinstances. The*wildcard now only refers to factories present in the provided map.Added the following methods to
EncodingService:These methods allow setting the factories that
EncodingServicewill pass toHttpEncoderwhen determining the appropriate factory from theAccept-Encodingheader.Added following methods to
EncoderServiceBuilder:Result
Closes #6390. Users can now explicitly control which
StreamEncoderFactoryinstancesEncodingServicewill use for eachAccept-Encodingheader value.Usage example: