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

[3.1] Jetty 12 HTTP/2 support #5481

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Conversation

senivam
Copy link
Contributor

@senivam senivam commented Nov 29, 2023

No description provided.

@senivam senivam self-assigned this Nov 30, 2023
@@ -0,0 +1,60 @@
/*
Copy link
Contributor

@jansupol jansupol Dec 6, 2023

Choose a reason for hiding this comment

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

java11 folder does not seem necessary. It can be put among the other files in the java folder. That would make the build (pom file) simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it will work as expected. Because in the case of the JettyHttp2ClientSupplier.java in the separate src folder the resulting jar will look like:

META-INF/versions/<JDK-version>/<package>/JettyHttp2ClientSupplier.java

and for related JDK a proper version will be picked up. However, if the JettyHttp2ClientSupplier.java would be in the main src folder it will be located directly in the compiled classes folder org.glassfish.jersey.jetty.http2.container (without version separation), and JDK probably won't be able to overwrite it by picking another JettyHttp2ClientSupplier.java for a particular JDK version (17+ in our case)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not how it works. The classloader first checks the META-INF versions and the classes afterwards. See for instance core-common in 2.x. It works for JDK 11, there is no META-INF/versions/8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@senivam senivam Dec 7, 2023

Choose a reason for hiding this comment

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

Yeah, I've got why it shall be here - if we compile 2 similar classes within the same cycle. The compile phase will fail. It is required to exclude from compiling either one or another version of the same class file. It's not possible to compile both files within the same name in a similar compile phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of JDK 17+ and no version separation we will have 2 similar class names in one compile phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, the resulting jar jersey-jetty-http2-connector.jar does not have the versions/11 folder as well, only versions/17 but sources structure shall be kept for compilation reasons.

Copy link
Contributor

@jansupol jansupol left a comment

Choose a reason for hiding this comment

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

Can the bom.pom be also updated?

@@ -0,0 +1,36 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

again java11 folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my first comment

Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
@jansupol jansupol merged commit 8a915c2 into eclipse-ee4j:3.1 Dec 8, 2023
3 checks passed
@senivam senivam added this to the 3.1.5 milestone Dec 8, 2023
@senivam senivam deleted the 31_http2_jetty_12 branch December 8, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants