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

fix #292 refactor gzip compression handler #297

Merged
merged 3 commits into from
Feb 27, 2018

Conversation

smaldini
Copy link
Contributor

In this PR, we have tried to refactor CompressionHandler to correctly aggregate bytes until threshold. Unfortunately it is now put on hold as it still presents stability issues that we eventually will identify in a subsequent PR.
Now the PR introduces a new simple CompressionHandler that will compress without minimum threshold all traffic, HttpServers can choose to opt-in/opt-on the globally configured default on a per-response basis by using HttpServerResponse#compression(boolean).

Stephane Maldini added 3 commits February 27, 2018 01:22
Previously the logic was not updating the buffer pending bytes.
Thus would prevent flush logic to operate correctly. Also
A convoluted FilteringCompressor was used.
Add HttpServerResponse#compression to control per response compression.
We don't need to wrap non reactor types, and we need to leave callable
source takes precedence
@smaldini smaldini added this to the 0.7.5.RELEASE milestone Feb 27, 2018
@codecov-io
Copy link

codecov-io commented Feb 27, 2018

Codecov Report

Merging #297 into master will decrease coverage by 1.18%.
The diff coverage is 38.2%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #297      +/-   ##
============================================
- Coverage     68.11%   66.92%   -1.19%     
+ Complexity     1030     1012      -18     
============================================
  Files            74       74              
  Lines          4318     4332      +14     
  Branches        615      615              
============================================
- Hits           2941     2899      -42     
- Misses         1006     1065      +59     
+ Partials        371      368       -3
Impacted Files Coverage Δ Complexity Δ
...ctor/ipc/netty/http/server/HttpServerResponse.java 44.44% <ø> (ø) 3 <0> (ø) ⬇️
...ttp/server/MinimumThresholdCompressionHandler.java 0% <0%> (ø) 0 <0> (?)
...java/reactor/ipc/netty/http/server/HttpServer.java 94.44% <100%> (+0.07%) 15 <0> (ø) ⬇️
src/main/java/reactor/ipc/netty/NettyOutbound.java 81.96% <100%> (-0.58%) 27 <2> (-1)
...pc/netty/http/server/SimpleCompressionHandler.java 100% <100%> (ø) 4 <4> (?)
...or/ipc/netty/http/server/HttpServerOperations.java 70.43% <75%> (+0.31%) 54 <2> (+2) ⬆️
.../main/java/reactor/ipc/netty/PublisherContext.java 73.91% <76.47%> (+1.18%) 5 <2> (+2) ⬆️
.../ipc/netty/http/client/HttpClientWSOperations.java 74.66% <0%> (-2.67%) 18% <0%> (-1%)
...java/reactor/ipc/netty/channel/ContextHandler.java 71.56% <0%> (-1.97%) 26% <0%> (-1%)
...or/ipc/netty/channel/ChannelOperationsHandler.java 64.1% <0%> (+0.69%) 59% <0%> (+1%) ⬆️
... and 3 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 6ccc1d6...b68bcca. Read the comment docs.

@smaldini smaldini added the type/bug A general bug label Feb 27, 2018
@smaldini smaldini changed the title 292 refactor gzip compression handler fix #292 refactor gzip compression handler Feb 27, 2018
@smaldini smaldini merged commit 4960c0f into master Feb 27, 2018
@smaldini smaldini deleted the 292-refactorGzipCompressionHandler branch February 27, 2018 20:22
.subscriberContext(context);
}

static <T, V> Publisher<V> publiserOrScalarMap(Publisher<T> publisher,
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in "publiser"...

Copy link
Member

Choose a reason for hiding this comment

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

Fixed thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants