Introduce BrotliEncoder by hyperxpro · Pull Request #11256 · netty/netty

hyperxpro · 2021-05-14T14:36:53Z

Motivation:
Currently, Netty only has BrotliDecoder which can decode Brotli encoded data. However, BrotliEncoder is missing which will encode normal data to Brotli encoded data.

Modification:
Added BrotliEncoder

Result:
Fixes #6899.

codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java

hyperxpro · 2021-05-14T15:02:02Z

\cc @slandelle @idelpivnitskiy

normanmaurer

Also please add tests

codec/src/main/java/io/netty/handler/codec/compression/Brotli.java

codec/src/main/java/io/netty/handler/codec/compression/BrotliDecoder.java

codec/src/main/java/io/netty/handler/codec/compression/BrotliEncoder.java

codec/src/test/java/io/netty/handler/codec/compression/BrotliDecoderTest.java

codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java

Fix failing test Revert IDE formatting Revert IDE formatting fixes fix test Revert more

slandelle · 2021-05-14T19:02:09Z

@hyperxpro Is brotli compression really supposed to be performed on the fly? From what I've experienced so far, compression seems to be really slow and best performed once and for all for static content, then cached.

hyperxpro · 2021-05-16T16:05:59Z

@slandelle I agree with your point but Brotli compression speed cost is compensated by the compression ratio. So yeah, we can have it.

codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java

idelpivnitskiy

Thank you for the PR @hyperxpro!

+1 for adding tests. For BrotliEncoder it's enough to add a subclass of AbstractEncoderTest. See LzfEncoderTest as an example. But we also need tests for new features of HttpContentCompressor and CompressorHttp2ConnectionEncoder.

Please, also add reference to brotli4j in NOTICE.txt file, and add LICENSE.brotli4j.txt file in license folder. We forgot to do that when we added decoder.

codec/src/main/java/io/netty/handler/codec/compression/BrotliEncoder.java

codec-http2/src/main/java/io/netty/handler/codec/http2/CompressorHttp2ConnectionEncoder.java

hyperxpro · 2021-05-30T15:03:20Z

I will add tests once you guys are okay with new commits.

idelpivnitskiy

BrotliEncoder and its test class LGTM! I will leave HTTP-related classes for others to review as I don't have much context on it.

Please, also add tests for HTTP with "br" compression to cover BrotliContentCompressor and CompressorHttp2ConnectionEncoder.

codec/src/main/java/io/netty/handler/codec/compression/BrotliEncoder.java

codec-http/src/main/java/io/netty/handler/codec/http/BrotliContentCompressor.java

hyperxpro · 2021-06-03T04:06:19Z

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:testCompile (default-testCompile) on project netty-codec: Compilation failure: Compilation failure: 
Error:  /home/runner/work/netty/netty/codec/src/test/java/io/netty/handler/codec/compression/BrotliEncoderTest.java:[23,7] error: BrotliEncoderTest is not abstract and does not override abstract method createChannel() in AbstractEncoderTest
Error:  /home/runner/work/netty/netty/codec/src/test/java/io/netty/handler/codec/compression/BrotliEncoderTest.java:[26,16] error: initChannel() in BrotliEncoderTest cannot override initChannel() in AbstractEncoderTest
Error:    overridden method is final

@normanmaurer Can you help? The test is same as LzfEncoderTest but it's not compiling.

kashike · 2021-06-03T04:19:20Z

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:testCompile (default-testCompile) on project netty-codec: Compilation failure: Compilation failure: 
Error:  /home/runner/work/netty/netty/codec/src/test/java/io/netty/handler/codec/compression/BrotliEncoderTest.java:[23,7] error: BrotliEncoderTest is not abstract and does not override abstract method createChannel() in AbstractEncoderTest
Error:  /home/runner/work/netty/netty/codec/src/test/java/io/netty/handler/codec/compression/BrotliEncoderTest.java:[26,16] error: initChannel() in BrotliEncoderTest cannot override initChannel() in AbstractEncoderTest
Error:    overridden method is final

@normanmaurer Can you help? The test is same as LzfEncoderTest but it's not compiling.

@hyperxpro
https://github.com/netty/netty/pull/11256/files#diff-129844c472b5cf087ae4119467312848077644c9e11f52563e0659d92d5c0df6R25-R28 should be:

            @Override
            protected EmbeddedChannel createChannel() {
                return new EmbeddedChannel(new BrotliEncoder());
            }

ref: https://github.com/netty/netty/blob/master/codec/src/test/java/io/netty/handler/codec/compression/LzfEncoderTest.java#L25-L28

normanmaurer · 2021-07-06T12:18:37Z

codec-http/src/main/java/io/netty/handler/codec/http/compression/CompressionOptions.java

+ * A {@link CompressionOptions} instance is thread-safe
+ * and should be shared between multiple instances of Compressor.
+ */
+public interface CompressionOptions {


I wonder if we should maybe move this to the codec module under the compression package... @trustin WDYT ?

hyperxpro · 2021-07-06T16:41:36Z

Actual: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbccccccccccccccccccccccc
Got:    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbccccccccccccccccccccccc

Never seen something this strange. @normanmaurer Any idea?

idelpivnitskiy

I like new style of CompressionOptions

codec-http2/src/main/java/io/netty/handler/codec/http2/CompressorHttp2ConnectionEncoder.java

hyperxpro · 2021-07-07T15:28:54Z

It's in the last call.

The compression bug is fixed (by using TEXT mode).

normanmaurer · 2021-07-08T08:48:50Z

@hyperxpro PTAL ... I think moving the options make more sense

hyperxpro · 2021-07-08T09:14:46Z

@hyperxpro PTAL ... I think moving the options make more sense

The API looks amazing. Thanks for the change. :)

normanmaurer · 2021-07-08T09:51:36Z

@hyperxpro thanks a lot!

hyperxpro · 2021-07-08T09:52:56Z

Thanks a lot to everyone who participated in this PR and gave their feedback. :)

Motivation: Currently, Netty only has BrotliDecoder which can decode Brotli encoded data. However, BrotliEncoder is missing which will encode normal data to Brotli encoded data. Modification: Added BrotliEncoder and CompressionOption Result: Fixes #6899. Co-authored-by: Norman Maurer <norman_maurer@apple.com>

trustin · 2021-07-08T12:58:49Z

codec/src/main/java/io/netty/handler/codec/compression/StandardCompressionOptions.java

+    }
+
+    /**
+     * @see BrotliOptions#DEFAULT


This, as well as other member references in this class's Javadoc, won't be linked properly in generated Javadoc because they are not public. We need to write a proper first sentence, such as:

* Returns the {@link BrotliOptions} with the default compression settings.

@trustin want to do a PR ?

I will do it as I've upcoming PR to enable Brotli tests on Windows.

Thanks, @hyperxpro 🙇

trustin · 2021-07-08T12:59:40Z

Thanks a lot for your hard work, @hyperxpro! Looking great indeed. Too bad we're not at Java 8 yet, though 😅

…11482) Motivation: In #11256, We introduced `Iterable` as a parameter but later in review, it was removed. But we forgot to change `compressionOptionsIterable` to just `compressionOptions`. Modification: Changed `compressionOptionsIterable` to `compressionOptions`. Result: Correct ObjectUtil message

Motivation: Currently, Netty only has BrotliDecoder which can decode Brotli encoded data. However, BrotliEncoder is missing which will encode normal data to Brotli encoded data. Modification: Added BrotliEncoder and CompressionOption Result: Fixes netty#6899. Co-authored-by: Norman Maurer <norman_maurer@apple.com>

…etty#11482) Motivation: In netty#11256, We introduced `Iterable` as a parameter but later in review, it was removed. But we forgot to change `compressionOptionsIterable` to just `compressionOptions`. Modification: Changed `compressionOptionsIterable` to `compressionOptions`. Result: Correct ObjectUtil message

…` license ### What changes were proposed in this pull request? This PR aims to update `NOTICE-binary` with `Netty` 4.2.7 license. - https://github.com/netty/netty/blob/netty-4.2.7.Final/NOTICE.txt ### Why are the changes needed? It seems that we updated `Netty Notice` at Apache Spark `3.0.0-preview` with `Netty 4.1.30.Final`. - #25544 Since there are many changes like the following, we need to update it by simply copying and pasting. - Netty 4.1.38.Final - netty/netty#9344 - Netty 4.1.44.Final - netty/netty#9161 - Netty 4.1.54.Final - netty/netty#10773 - Netty 4.1.66.Final - netty/netty#11256 - netty/netty#11437 - Netty 4.1.108.Final - netty/netty#13864 - Netty 4.2.1.Final - netty/netty#14979 - Netty 4.2.7.Final - netty/netty#15658 Additionally, I also double-checked newly added transitive license through ASF [LEGAL-700](https://issues.apache.org/jira/browse/LEGAL-700). We are good to go. - **Apple Public Source License 2.0** - https://spdx.org/licenses/APSL-2.0.html ### Does this PR introduce _any_ user-facing change? No behavior change. ### How was this patch tested? Manual review. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #53335 from dongjoon-hyun/SPARK-54602. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…` license ### What changes were proposed in this pull request? This PR aims to update `NOTICE-binary` with `Netty` 4.2.7 license. - https://github.com/netty/netty/blob/netty-4.2.7.Final/NOTICE.txt ### Why are the changes needed? It seems that we updated `Netty Notice` at Apache Spark `3.0.0-preview` with `Netty 4.1.30.Final`. - #25544 Since there are many changes like the following, we need to update it by simply copying and pasting. - Netty 4.1.38.Final - netty/netty#9344 - Netty 4.1.44.Final - netty/netty#9161 - Netty 4.1.54.Final - netty/netty#10773 - Netty 4.1.66.Final - netty/netty#11256 - netty/netty#11437 - Netty 4.1.108.Final - netty/netty#13864 - Netty 4.2.1.Final - netty/netty#14979 - Netty 4.2.7.Final - netty/netty#15658 Additionally, I also double-checked newly added transitive license through ASF [LEGAL-700](https://issues.apache.org/jira/browse/LEGAL-700). We are good to go. - **Apple Public Source License 2.0** - https://spdx.org/licenses/APSL-2.0.html ### Does this PR introduce _any_ user-facing change? No behavior change. ### How was this patch tested? Manual review. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #53335 from dongjoon-hyun/SPARK-54602. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 191ce4c) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

normanmaurer requested changes May 14, 2021

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java Outdated Show resolved Hide resolved

codec-http/src/main/java/io/netty/handler/codec/http/HttpContentCompressor.java Outdated Show resolved Hide resolved

hyperxpro force-pushed the brotli branch 2 times, most recently from 0adae1b to faa0621 Compare May 14, 2021 14:59

normanmaurer requested changes May 14, 2021

View reviewed changes

hyperxpro force-pushed the brotli branch from faa0621 to 0a834ae Compare May 14, 2021 15:09

idelpivnitskiy self-requested a review May 14, 2021 15:30

hyperxpro force-pushed the brotli branch from 0a834ae to 9ebf657 Compare May 14, 2021 16:00

Introduce BrotliEncoder

c0482d5

Fix failing test Revert IDE formatting Revert IDE formatting fixes fix test Revert more

hyperxpro force-pushed the brotli branch from 9ebf657 to c0482d5 Compare May 14, 2021 16:11

normanmaurer requested changes May 17, 2021

View reviewed changes

normanmaurer modified the milestones: 4.1.65.Final, 4.1.66.Final May 17, 2021

idelpivnitskiy requested changes May 26, 2021

View reviewed changes

hyperxpro added 4 commits May 30, 2021 19:25

Revert back

d457557

Add BrotliContentCompressor

eae04d6

Add compression support in CompressorHttp2ConnectionEncoder

d3395e0

use https links

c5e8710

hyperxpro requested review from idelpivnitskiy and normanmaurer May 30, 2021 15:03

hyperxpro added 2 commits June 1, 2021 12:04

Add tests

8d4ed1d

Add copyright header

4944937

idelpivnitskiy reviewed Jun 2, 2021

View reviewed changes

address comments

81bc354

explicitly define Charset name

3067570

normanmaurer reviewed Jul 6, 2021

View reviewed changes

use brotli text mode

ad4ce6f

idelpivnitskiy approved these changes Jul 7, 2021

View reviewed changes

codec-http2/src/main/java/io/netty/handler/codec/http2/CompressorHttp2ConnectionEncoder.java Outdated Show resolved Hide resolved

codec-http2/src/main/java/io/netty/handler/codec/http2/CompressorHttp2ConnectionEncoder.java Show resolved Hide resolved

hyperxpro added 6 commits July 7, 2021 11:51

Use new API in CompressorHttp2ConnectionEncoder

dd81a28

Add JavaDoc and deprecate old constructors

70c06f6

Remove Iterable

bc45114

fix compile error

48e669b

fix checkstyle

6170d7a

use TEXT mode in encoder

a8bc078

move options to codec.compression package

aca20cf

normanmaurer merged commit fef761d into netty:4.1 Jul 8, 2021

hyperxpro deleted the brotli branch July 8, 2021 09:52

trustin reviewed Jul 8, 2021

View reviewed changes

hyperxpro mentioned this pull request Jul 12, 2021

Fix ObjectUtil message in HttpContentCompressor #11482

Merged

dongjoon-hyun mentioned this pull request Dec 5, 2025

[SPARK-54602][BUILD] Update NOTICE-binary with Netty 4.2.7.Final license apache/spark#53335

Closed

Uh oh!

Conversation

hyperxpro commented May 14, 2021

Uh oh!

Uh oh!

Uh oh!

hyperxpro commented May 14, 2021

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slandelle commented May 14, 2021

Uh oh!

hyperxpro commented May 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hyperxpro commented May 30, 2021

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hyperxpro commented Jun 3, 2021

Uh oh!

kashike commented Jun 3, 2021

Uh oh!

normanmaurer Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

hyperxpro commented Jul 6, 2021

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hyperxpro commented Jul 7, 2021

Uh oh!

normanmaurer commented Jul 8, 2021

Uh oh!

hyperxpro commented Jul 8, 2021

Uh oh!

normanmaurer commented Jul 8, 2021

Uh oh!

hyperxpro commented Jul 8, 2021

Uh oh!

trustin Jul 8, 2021 • 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!

normanmaurer Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

hyperxpro Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

trustin Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

trustin commented Jul 8, 2021

Uh oh!

trustin Jul 8, 2021 •

edited

Loading