Adds Brotli compression support for HTTP (via libbrotli) #40750

Karm · 2024-05-21T17:03:38Z

fixes #40692
relates to #40533

Tested Platforms Checklist ✔️

Ready for review.

quarkus-bot · 2024-05-21T17:03:59Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should preferably start with an uppercase character (if it makes sense!)

_{This message is automatically generated by a bot.}

github-actions · 2024-05-21T18:28:06Z

🙈 The PR is closed and the preview is expired.

Karm · 2024-05-21T20:09:24Z

The following artifacts could not be resolved: io.quarkus:quarkus-integration-test-vertx-http-compressors-parent:pom:999-SNAPSHOT

I see, I have to fix the pom structure. I have the parent already installed locally.

cescoffier

LGTM!

...http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java

Karm · 2024-05-22T11:49:18Z

2024-05-22T11:42:29.0092701Z [INFO] asciidoctor: ERROR: telemetry-opentracing-to-otel-tutorial.adoc: line 306: dropping cells from incomplete row detected end of table
2024-05-22T11:42:29.0582466Z [INFO] Converted /home/runner/work/quarkus/quarkus/docs/target/asciidoc/sources/telemetry-opentracing-to-otel-tutorial.adoc
2024-05-22T11:42:29.0585271Z [ERROR] asciidoctor: ERROR: telemetry-opentracing-to-otel-tutorial.adoc: line 306: dropping cells from incomplete row detected end of table

Documentation error is unrelated to my PR. An error was merged with another PR.

Karm · 2024-05-22T11:50:52Z

Documentation update preview:

https://quarkus-pr-main-40750-preview.surge.sh/version/main/guides/http-reference#http-compression

Karm · 2024-05-23T12:22:20Z

@zakkak Thx, commentary added 2ae2097.

Karm · 2024-05-23T20:23:25Z

@cescoffier This can be merged.

Karm · 2024-06-04T09:41:00Z

Oh my oh my.

This branch has conflicts that must be resolved

This PR has been here for so long that apart from rebases there are conflicts now.

As there is no new material feedback, I will merge it once I resolve the conflicts.

Karm · 2024-06-04T12:44:57Z

Ad Data 7 module: #40960

gsmet · 2024-06-04T16:20:56Z

...s/vertx-http-compressors/some/src/test/java/io/quarkus/compressors/it/RESTEndpointsTest.java

+    "/yes/text | deflate,gzip,br | gzip             | 2414",
+    "/yes/text | deflate         | deflate          | 2402",
+    "/no/text  | deflate,gzip,br | null             | 6483",
+    "/yes/json | deflate         | deflate          | 2402",
+    "/no/json  | deflate,gzip,br | null             | 6483",
+    "/yes/xml  | deflate,gzip,br | gzip             | 2414",
+    "/no/xml   | deflate,gzip,br | null             | 6483",


Again, can't we check a marker in the content instead? That's what I originally asked for and I don't want to be in the situation we already encountered with AWT and other things, where we have to update numeric values when a new implementation of Brotli appears.

Karm · 2024-06-04T17:23:40Z

@gsmet

Again, can't we check a marker in the content instead? That's what I originally asked for

Checking the magic number at the beginning of the received file does not test that the compression took place as we expected, using the parameters we set. Note in the code that I am not comparing as ==, test does <=, so nothing goes south if the compression ratio gets magically better. If we suddenly botch passing parameters to Vert.x -> Netty, the compression would get worse result and fail.

I excruciatingly tested your hypothesis by testing numerous old versions across platforms. I did find that a very old Vert.x indeed showed 1 byte difference, better in fact, compression result. I accounted for this by adding a tolerance of 2 bytes.

I this PR, we have our webserver vendor's hats on and we are testing a web server. That's why I manipulate headers in the test as that is what webserver test must do correctly.

I don't want to be checking a magic number at the beginning of a binary and calling it a day, I want to see that the whole exchange makes sense and the compression works as expected.

When I add dealing with chunked responses, this will get even more pertinent.

and I don't want to be in the situation we already encountered with AWT and other things, where we have to update numeric values when a new implementation of Brotli appears.

Can you point to a single trouble with vastly more complex AWT test suite I hadn't fixed in a timely fashion? I maintain the tests I commit.

I had to refresh my memory, and I would like to add that the moment where pixel ranges suddenly didn't match was actually caused by the native-image using a different library for fonts processing, JDK in-tree built one versus the OS one. To learn about this change from the test suite is not without value. A change in JDK, given the AWT extension's nature, is again very valuable to learn about.

Bringing the argument home, if the compression ratio gets worse (e.g. by not passing correct compression level arguments), we want to know about it.

Feel free to merge.

geoand · 2024-06-06T13:31:29Z

Let's at least please up the commits and rebase onto the latest main

maxandersen · 2024-06-06T13:46:56Z

...ation-tests/vertx-http-compressors/app/src/test/java/io/quarkus/compressors/it/Testflow.java

+    // Depending on the defaults of the compression algorithm,
+    // the compressed content length may vary slightly between
+    // Vert.x/Netty versions over time.
+    public static final int COMPRESSION_TOLERANCE_BYTES = 2;


2 bytes is quite small for a 5x factor compression. would 2% tolerance be better / less likely to break ?

Yes, having a reasonable tolerance window would mitigate the potential issue and still be accurate enough.

Implemented in cc361d1.

gsmet · 2024-06-06T14:06:12Z

@Karm FWIW, there was nothing personal in my comments.

I completely agree that you maintain the AWT tests and that you do it in a timely manner. But what if you are on PTO for 3 weeks or have more pressing things to do? We need to find a balance between being thorough and being brittle. And also how failures are actionable and by who. Because you, as any other developer, have the right to not be available to fix a test in the Quarkus test suite.

Now, if these actually allowed to catch important things to fix/adjust then it's all fine. My perception is that you had on a few occasions to spend some time maintaining some test values because of changes that were completely out of our control. Or because we wanted to test another platform/Java version/...

In this very case, we are testing a compression library that is out of our control. It will probably improve over time but really, there might be cases where it gets slightly worse for specific inputs for whatever reason (while improving overall).

If you want to make sure that a particular set of settings is taken into account then fine, but I'm not entirely sure testing the size is going to bring you that. Let's say they improve things well enough that even without your settings applied, you are below the threshold?

Anyway, what I wanted to emphasize is that it's not because it's fine now, that it will be fine along the life of Quarkus and the fact that Brotli compresses our text with 3 more bytes have very little value to us. And definitely not enough value for us to break our test suite.
FWIW, I didn't expect you to run tests on a gazillion of versions and sorry if you thought that's what I was asking. Because my problem is not Brotli on June 6th 2024, it's how to make sure Quarkus test suite remains sustainable in the long run if we follow this path for too many components.

I dismissed my review so feel free to merge this PR but I would still have a look at Max' comment, which results from a discussion we had just now.
It would make things less brittle while, I think, still testing what you want to test (or at least be an acceptable approximation).

* Integration tests for compressors * Adds Native image integration tests modules

… at compressing our example to trigger a failure.

Karm · 2024-06-10T13:45:55Z

@Karm FWIW, there was nothing personal in my comments.

I completely agree that you maintain the AWT tests and that you do it in a timely manner. But what if you are on PTO for 3 weeks or have more pressing things to do? We need to find a balance between being thorough and being brittle. And also how failures are actionable and by who. Because you, as any other developer, have the right to not be available to fix a test in the Quarkus test suite.

Now, if these actually allowed to catch important things to fix/adjust then it's all fine. My perception is that you had on a few occasions to spend some time maintaining some test values because of changes that were completely out of our control. Or because we wanted to test another platform/Java version/...

In this very case, we are testing a compression library that is out of our control. It will probably improve over time but really, there might be cases where it gets slightly worse for specific inputs for whatever reason (while improving overall).

If you want to make sure that a particular set of settings is taken into account then fine, but I'm not entirely sure testing the size is going to bring you that. Let's say they improve things well enough that even without your settings applied, you are below the threshold?

Anyway, what I wanted to emphasize is that it's not because it's fine now, that it will be fine along the life of Quarkus and the fact that Brotli compresses our text with 3 more bytes have very little value to us. And definitely not enough value for us to break our test suite. FWIW, I didn't expect you to run tests on a gazillion of versions and sorry if you thought that's what I was asking. Because my problem is not Brotli on June 6th 2024, it's how to make sure Quarkus test suite remains sustainable in the long run if we follow this path for too many components.

I dismissed my review so feel free to merge this PR but I would still have a look at Max' comment, which results from a discussion we had just now. It would make things less brittle while, I think, still testing what you want to test (or at least be an acceptable approximation).

Thx for the additional explanation and especially for the time spent on the case. I appreciate it.

FWIW, I didn't expect you to run tests on a gazillion of versions and sorry if you thought that's what I was asking.

That is on me. I was curious how stable those implementations behave. Whether they get any worse or better between versions historically.

@gsmet @maxandersen @cescoffier

If for some reason the compressor gets wayyy worse, this is how the failure looks like:

 AssertionFailedError: Compression apparently failed: receivedLength: 2316 was
supposed to be less or equal to expectedLength: 1316 plus 2% tolerance, i.e. 1342. 
==> expected: <true> but was: <false>

quarkus-bot · 2024-06-10T14:00:12Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit cc361d1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

maxandersen

looks good with the 2% threshold.

quarkus-bot · 2024-06-10T19:01:48Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit cc361d1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/opentelemetry/deployment

✖ io.quarkus.opentelemetry.deployment.OpenTelemetryServiceNameCombinedResourceWinsTest.testServiceName - History

event executor terminated - java.util.concurrent.RejectedExecutionException

java.util.concurrent.RejectedExecutionException: event executor terminated
	at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:934)
	at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:351)
	at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:344)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:836)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute0(SingleThreadEventExecutor.java:827)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:817)
	at io.vertx.core.impl.EventLoopExecutor.execute(EventLoopExecutor.java:35)

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

⚙️ JVM Tests - JDK 21

📦 extensions/infinispan-cache/deployment

✖ io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:499)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:413)

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 integration-tests/reactive-messaging-kafka

✖ io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

Karm requested review from galderz, cescoffier and radcortez May 21, 2024 17:03

Karm self-assigned this May 21, 2024

Karm marked this pull request as draft May 21, 2024 17:03

quarkus-bot bot added area/documentation area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/rest area/vertx labels May 21, 2024

This comment has been minimized.

Sign in to view

Karm force-pushed the brotli-4-all branch from ace5448 to b4fa5e7 Compare May 21, 2024 23:22

cescoffier reviewed May 22, 2024

View reviewed changes

...http/runtime/src/main/java/io/quarkus/vertx/http/runtime/options/HttpServerOptionsUtils.java Outdated Show resolved Hide resolved

Karm changed the title ~~[WIP] Adds Brotli compression support for HTTP [WIP]~~ Adds Brotli compression support for HTTP (via libbrotli) May 22, 2024

Karm marked this pull request as ready for review May 22, 2024 11:47

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label May 22, 2024

radcortez approved these changes May 23, 2024

View reviewed changes

Karm force-pushed the brotli-4-all branch from 320ebc1 to 2ae2097 Compare May 23, 2024 12:21

This comment has been minimized.

Sign in to view

Karm force-pushed the brotli-4-all branch from 23ae7ed to e534a94 Compare June 4, 2024 10:15

This comment has been minimized.

Sign in to view

gsmet previously requested changes Jun 4, 2024

View reviewed changes

Karm removed the triage/flaky-test label Jun 5, 2024

maxandersen reviewed Jun 6, 2024

View reviewed changes

Karm added 2 commits June 10, 2024 09:43

Adds Brotli support via native libbrotli

b41b6ed

* Integration tests for compressors * Adds Native image integration tests modules

Adds 2% tolerance, i.e. compressor would have to get 2% or more WORSE…

cc361d1

… at compressing our example to trigger a failure.

Karm force-pushed the brotli-4-all branch from e534a94 to cc361d1 Compare June 10, 2024 13:32

maxandersen approved these changes Jun 10, 2024

View reviewed changes

Karm mentioned this pull request Jun 10, 2024

Vert.x Http: application/json MIME not compressed by default #41094

Closed

quarkus-bot bot added the triage/flaky-test label Jun 10, 2024

gsmet approved these changes Jun 10, 2024

View reviewed changes

gsmet merged commit 169608b into quarkusio:main Jun 10, 2024
55 checks passed

quarkus-bot bot added this to the 3.12 - main milestone Jun 10, 2024

quarkus-bot bot added the kind/enhancement New feature or request label Jun 10, 2024

Karm mentioned this pull request Sep 26, 2024

Warnings about brotli in quarkus-netty when compiled with Mandrel 24.2 (JDK 24+15) or better #43538

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Brotli compression support for HTTP (via libbrotli) #40750

Adds Brotli compression support for HTTP (via libbrotli) #40750

Karm commented May 21, 2024 •

edited

Loading

quarkus-bot bot commented May 21, 2024

github-actions bot commented May 21, 2024 •

edited

Loading

Karm commented May 21, 2024 •

edited

Loading

This comment has been minimized.

cescoffier left a comment

This comment has been minimized.

Karm commented May 22, 2024

Karm commented May 22, 2024

This comment has been minimized.

Karm commented May 23, 2024

This comment has been minimized.

This comment has been minimized.

Karm commented May 23, 2024

This comment has been minimized.

This comment has been minimized.

Karm commented Jun 4, 2024

This comment has been minimized.

Karm commented Jun 4, 2024

This comment has been minimized.

gsmet Jun 4, 2024

Karm commented Jun 4, 2024

geoand commented Jun 6, 2024

maxandersen Jun 6, 2024

cescoffier Jun 6, 2024

Karm Jun 10, 2024

gsmet commented Jun 6, 2024 •

edited

Loading

Karm commented Jun 10, 2024

quarkus-bot bot commented Jun 10, 2024

maxandersen left a comment

quarkus-bot bot commented Jun 10, 2024 •

edited by github-actions bot

Loading

Adds Brotli compression support for HTTP (via libbrotli) #40750

Adds Brotli compression support for HTTP (via libbrotli) #40750

Conversation

Karm commented May 21, 2024 • edited Loading

Tested Platforms Checklist ✔️

quarkus-bot bot commented May 21, 2024

github-actions bot commented May 21, 2024 • edited Loading

Karm commented May 21, 2024 • edited Loading

This comment has been minimized.

cescoffier left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Karm commented May 22, 2024

Karm commented May 22, 2024

This comment has been minimized.

Karm commented May 23, 2024

This comment has been minimized.

This comment has been minimized.

Karm commented May 23, 2024

This comment has been minimized.

This comment has been minimized.

Karm commented Jun 4, 2024

This comment has been minimized.

Karm commented Jun 4, 2024

This comment has been minimized.

gsmet Jun 4, 2024

Choose a reason for hiding this comment

Karm commented Jun 4, 2024

geoand commented Jun 6, 2024

maxandersen Jun 6, 2024

Choose a reason for hiding this comment

cescoffier Jun 6, 2024

Choose a reason for hiding this comment

Karm Jun 10, 2024

Choose a reason for hiding this comment

gsmet commented Jun 6, 2024 • edited Loading

Karm commented Jun 10, 2024

quarkus-bot bot commented Jun 10, 2024

Status for workflow Quarkus Documentation CI

maxandersen left a comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jun 10, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/opentelemetry/deployment

📦 extensions/smallrye-reactive-messaging-kafka/deployment

⚙️ JVM Tests - JDK 21

📦 extensions/infinispan-cache/deployment

📦 extensions/smallrye-reactive-messaging-kafka/deployment

📦 integration-tests/reactive-messaging-kafka

Karm commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024 •

edited

Loading

Karm commented May 21, 2024 •

edited

Loading

gsmet commented Jun 6, 2024 •

edited

Loading

Status for workflow `Quarkus Documentation CI`

quarkus-bot bot commented Jun 10, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`