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

Add Brotli4J compression related classes, files and dependencies #1717

Merged
merged 1 commit into from
May 9, 2024
Merged

Add Brotli4J compression related classes, files and dependencies #1717

merged 1 commit into from
May 9, 2024

Conversation

jcarranzan
Copy link
Contributor

@jcarranzan jcarranzan commented Mar 19, 2024 •
edited
Loading

Summary

Integration of Brotli4J compression tests with Quarkus REST endpoints.
Test Plan related to: [QQE-378](https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QQE-378.md)

Test Coverage:

JVM and native mode testing will be included.

Test Scenarios Covered:

  • Compression Enabled/Disabled:

  Confirms response compression with Brotli when enabled/disabled.

  • Media Types size Payloads:

  Verifies Brotli compression up to 8192 bytes for JSON payloads.
  Validates error handling for payloads larger than 8192 bytes.

  • Different Media Types of payload compression (text, JSON, XML):

  Checks compression correctness

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch 3 times, most recently from f3f5f05 to da67735 Compare March 20, 2024 10:52
@jcarranzan jcarranzan mentioned this pull request Mar 20, 2024
Merged
1 task
@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch 3 times, most recently from 9fd6151 to 5c90b81 Compare April 30, 2024 13:39
@jcarranzan jcarranzan marked this pull request as ready for review April 30, 2024 13:39
@jcarranzan jcarranzan requested a review from jedla97 April 30, 2024 13:40
jedla97
jedla97 requested changes Apr 30, 2024
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Thanks for adding coverage @jcarranzan. I have some comments most of them are minor.

Looking at the test plan I see that you plan to test it with openshift. Will this testcoverage part of this PR or it will be separate PR? I think it can be part of this PR but leave the decision up to you

Also can you modify commit message not to include squashed commit messages to keep it more clean.

...ed-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpServerConfig.java Outdated Show resolved Hide resolved
...p-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/Brotli4JResource.java Outdated Show resolved Hide resolved
...p-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/Brotli4JRestMock.java Outdated Show resolved Hide resolved
http/http-advanced-reactive/src/main/resources/json_sample.json Outdated Show resolved Hide resolved
...p-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/Brotli4JResource.java Outdated Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Outdated Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Outdated Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Outdated Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Outdated Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Outdated Show resolved Hide resolved
@jcarranzan
Copy link
Contributor Author

Thanks for adding coverage @jcarranzan. I have some comments most of them are minor.

Looking at the test plan I see that you plan to test it with openshift. Will this testcoverage part of this PR or it will be separate PR? I think it can be part of this PR but leave the decision up to you

Also can you modify commit message not to include squashed commit messages to keep it more clean.

Hi, yes I will include Openshift coverage in this PR, and also another test about big file compression (>8192 bytes), I 'll let you know it when is ready.
Also, I'll clean squashed commit message, thanks @jedla97 .

@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch 3 times, most recently from 27b637d to 05b23d3 Compare May 2, 2024 16:01
michalvavrik
michalvavrik reviewed May 2, 2024
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

  • You have marked This change requires a documentation update but there is no docs update. Please add docs update.

  • You have marked Dependency update, I'll respect your choice, but I think it's meant for a bump. I am not sure I'd call adding dep an update.

@jcarranzan jcarranzan marked this pull request as draft May 3, 2024 06:19
@jcarranzan
Copy link
Contributor Author

jcarranzan commented May 7, 2024 •
edited
Loading

  • The test module failed in Linux-JVM (17) is hibernate-reactive, so not related to this coverage.
  • The Openshift Brotli4J tests passed with JDK 11 but failed with Openshift native mode JDK 17, it seems there is some kind of problem in the brotli4J library side in native:
20:30:02  18:30:02,394 INFO  [app] 	at org.graalvm.nativeimage.builder/com.oracle.svm.core.jni.access.JNINativeLinkage.getOrFindEntryPoint(JNINativeLinkage.java:152)
20:30:02  18:30:02,394 INFO  [app] 	at org.graalvm.nativeimage.builder/com.oracle.svm.core.jni.JNIGeneratedMethodSupport.nativeCallAddress(JNIGeneratedMethodSupport.java:54)
20:30:02  18:30:02,394 INFO  [app] 	at com.aayushatharva.brotli4j.encoder.EncoderJNI.nativeCreate(Native Method)
20:30:02  18:30:02,394 INFO  [app] 	at com.aayushatharva.brotli4j.encoder.EncoderJNI.access$200(EncoderJNI.java:17)
20:30:02  18:30:02,394 INFO  [app] 	at com.aayushatharva.brotli4j.encoder.EncoderJNI$Wrapper.<init>(EncoderJNI.java:90)
20:30:02  18:30:02,394 INFO  [app] 	at com.aayushatharva.brotli4j.encoder.Encoder.<init>(Encoder.java:48)
20:30:02  18:30:02,394 INFO  [app] 	at com.aayushatharva.brotli4j.encoder.BrotliEncoderChannel.<init>(BrotliEncoderChannel.java:38)
20:30:02  18:30:02,394 INFO  [app] 	at com.aayushatharva.brotli4j.encoder.BrotliEncoderChannel.<init>(BrotliEncoderChannel.java:50)

On the other hand there are issues loading the files, so I'll investigate deeper if there is something in the code (like readAllBytes() method) that could be the root cause of this error.

jedla97
jedla97 requested changes May 7, 2024
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Thanks for updating all the coments overal it's loog good. I have just 3 small comments and one question

...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Show resolved Hide resolved
...anced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/DevModeBrotli4JHttpIT.java Show resolved Hide resolved
...ttp-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/Brotli4JHttpIT.java Outdated Show resolved Hide resolved
@michalvavrik
Copy link
Member

The Openshift Brotli4J tests passed with JDK 11 but failed with Openshift native mode JDK 17, it seems there is some kind of problem in the brotli4J library side in native:
On the other hand there are issues loading the files, so I'll investigate deeper if there is something in the code (like readAllBytes() method) that could be the root cause of this error.

Try https://quarkus.io/guides/all-config#quarkus-core_quarkus-native-resources-includes.

@michalvavrik
Copy link
Member

The test module failed in Linux-JVM (17) is hibernate-reactive, so not related to this coverage.

As of now. the failing tests should be disabled. Please rebase on the current main next time you push changes.

@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch 2 times, most recently from 08c87a7 to df63604 Compare May 7, 2024 09:42
@jcarranzan
Copy link
Contributor Author

run tests

@github-actions github-actions bot added the triage/flaky-test Signal that flaky tests were detected during CI run label May 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch 3 times, most recently from 183bf65 to 18c8d16 Compare May 8, 2024 10:07
@jcarranzan
Copy link
Contributor Author

run tests

@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch from fbe1cfd to e452bf4 Compare May 9, 2024 10:50
@jcarranzan
Copy link
Contributor Author

Issue related to native mode execution created here--> quarkusio/quarkus#40533

@jedla97
Copy link
Member

jedla97 commented May 9, 2024

@jcarranzan Looking at this it's loog good. Just one question as the quarkusio/quarkus#40533 is not just on openshift but also occuring on baremetal shouldn't be Brotli4JHttpIT also disabled on native?

I run it localy using mvn clean verify -Phttp-modules -Dfailsafe.failIfNoSpecifiedTests=false -pl http/http-advanced-reactive -Dreruns=0 -Dit.test="Brotli4JHttpIT" -Dnative and hitting the error. Looking at the our CI for native and it didn't execute any tests (don't know why) https://github.com/quarkus-qe/quarkus-test-suite/actions/runs/9016177799/job/24773143474?pr=1717

@jcarranzan
Copy link
Contributor Author

@jcarranzan Looking at this it's loog good. Just one question as the quarkusio/quarkus#40533 is not just on openshift but also occuring on baremetal shouldn't be Brotli4JHttpIT also disabled on native?

I run it localy using mvn clean verify -Phttp-modules -Dfailsafe.failIfNoSpecifiedTests=false -pl http/http-advanced-reactive -Dreruns=0 -Dit.test="Brotli4JHttpIT" -Dnative and hitting the error. Looking at the our CI for native and it didn't execute any tests (don't know why) https://github.com/quarkus-qe/quarkus-test-suite/actions/runs/9016177799/job/24773143474?pr=1717

Yes, it should be disabled also on baremetal in this case. I will push my changes right now.
On the other hand @jedla97 , IDK either why any tests were executed, maybe any external change in our ts could have affected this?

@jcarranzan
Add Brotli4J compression tests
57fb1ff 
disable on native mode

add @DisabledOnNative also on baremetal

add @DisabledOnNative also on baremetal
@jcarranzan jcarranzan force-pushed the brotli4j-coverage branch from 8278975 to 57fb1ff Compare May 9, 2024 13:42
@jedla97
Copy link
Member

jedla97 commented May 9, 2024

run tests

jedla97
jedla97 approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Thanks for update!. Now it look good.

@jedla97
Copy link
Member

jedla97 commented May 9, 2024

CI is green + native modules was executed this time so merging

@jedla97 jedla97 merged commit 112dd42 into quarkus-qe:main May 9, 2024
10 checks passed
@michalvavrik michalvavrik removed the triage/flaky-test Signal that flaky tests were detected during CI run label Nov 20, 2024
@michalvavrik michalvavrik deleted the brotli4j-coverage branch November 20, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalvavrik michalvavrik michalvavrik left review comments

@jedla97 jedla97 jedla97 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jcarranzan @michalvavrik @jedla97