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.

Sign up for GitHub

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

Jump to bottom

HTTP response compression - fix Undertow and RESTEasy Classsic support #31438

Merged
merged 1 commit into from
Mar 2, 2023
Merged

HTTP response compression - fix Undertow and RESTEasy Classsic support #31438

merged 1 commit into from
Mar 2, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Feb 27, 2023

@mkouba mkouba requested a review from geoand February 27, 2023 10:08
@mkouba mkouba requested a review from gsmet February 27, 2023 10:09
@github-actions
Copy link

github-actions bot commented Feb 27, 2023 •
edited
Loading

🙈 The PR is closed and the preview is expired.

gsmet
gsmet reviewed Feb 27, 2023
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Spotted a small typo. Otherwise looks good from what I can see.

...ions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/HttpBuildTimeConfig.java Outdated Show resolved Hide resolved
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 28, 2023
@mkouba mkouba requested a review from gsmet February 28, 2023 08:51
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Feb 28, 2023

@gsmet Do you have an idea why do the Quarkus CI / Maven Tests fail?

@geoand
Copy link
Contributor

geoand commented Feb 28, 2023

I've seen it happen in an other PR as well, but I have not had time to look into it.

@Sgitario as it is XML related, perhaps you can have a look?

@Sgitario
Copy link
Contributor

I've seen it happen in an other PR as well, but I have not had time to look into it.

@Sgitario as it is XML related, perhaps you can have a look?

The quarkus-jaxb extension is validating the JAXBContext is valid at build time and fails if it's not.
My suspicion is that the stax-api dependency brings some javax.xx classes that might the JAXB to fail as we now support Jakarta only.

I have no idea why we use the stax-api dependency as part of this test, any hints?

@geoand
Copy link
Contributor

geoand commented Feb 28, 2023

I have no idea why we use the stax-api dependency as part of this test, any hints?

None whatosever :) I have no idea what the test is doing or what it was added.

I am pretty sure that we can just remove it and not loose anything WRT test coverage if we can't quickly find a solution.

@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Feb 28, 2023

The CI failures do not seem to be related. I'll re-run the Devtools Tests - JDK 17 and if it passes I'd like to merge this pr.

@quarkus-bot

This comment has been minimized.

Sign in to view
@Sgitario
Copy link
Contributor

Sgitario commented Mar 1, 2023

I have no idea why we use the stax-api dependency as part of this test, any hints?

None whatosever :) I have no idea what the test is doing or what it was added.

I am pretty sure that we can just remove it and not loose anything WRT test coverage if we can't quickly find a solution.

Let's continue the discussion here: #31494

@mkouba
HTTP response compression - fix Undertow and RESTEasy Classsic support
5934e09 
- honor the quarkus.http.compress-media-types in Undertow Servlet and
RESTEasy Classsic extensions
- fixes quarkusio#31415 and quarkusio#26112
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 1, 2023

Failing Jobs - Building 5934e09

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
⌛ JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 19
✖ Native Tests - Misc4 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Misc4 #

- Failing: integration-tests/opentelemetry-jdbc-instrumentation

📦 integration-tests/opentelemetry-jdbc-instrumentation

✖ io.quarkus.it.opentelemetry.PostgresOpenTelemetryJdbcInstrumentationIT.testPostgreSqlQueryTraced - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

@mkouba
Copy link
Contributor Author

mkouba commented Mar 2, 2023

@gsmet It seems that the Native Tests - Misc4 is flaky, fails in some other PRs as well.

@gsmet gsmet merged commit 1ebe711 into quarkusio:main Mar 2, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Mar 2, 2023
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@melloware melloware melloware approved these changes

@stuartwdouglas stuartwdouglas Awaiting requested review from stuartwdouglas

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/documentation area/undertow area/vertx kind/bugfix
Projects
None yet
Milestone
3.0.0.Alpha5
Development

Successfully merging this pull request may close these issues.

GZIP Compression not working in servlet application
5 participants
@mkouba @geoand @Sgitario @gsmet @melloware