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

HTTP/2 for Jetty container #5296

Merged
merged 1 commit into from
May 2, 2023

Conversation

senivam
Copy link
Contributor

@senivam senivam commented Mar 24, 2023

No description provided.

@senivam senivam self-assigned this Mar 24, 2023
@senivam senivam marked this pull request as draft March 27, 2023 11:37
@senivam senivam marked this pull request as ready for review March 31, 2023 11:06
// Start the server.
server.start();
} catch (final Exception e) {
throw new ProcessingException(LocalizationMessages.ERROR_WHEN_CREATING_SERVER(), e);
Copy link
Contributor

@jansupol jansupol Apr 4, 2023

Choose a reason for hiding this comment

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

The localization-messages file is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's taken from the original Jetty container. Shall it be separated?

Copy link
Contributor

@jansupol jansupol Apr 4, 2023

Choose a reason for hiding this comment

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

The generated class searches for localization messages from the module the LocalizationMassages class is from. This module shares the package name with the original Jetty module, which is wrong, and likely there would be a Java warning (with JPMS at least) at some point about a package being in another module. Hence, it is not clear where the org.glassfish.jersey.jetty.internal.LocalizationMessages is from. But given you use

            <plugin>
                <groupId>com.sun.istack</groupId>
                <artifactId>istack-commons-maven-plugin</artifactId>
                <inherited>true</inherited>
            </plugin>
            <plugin>
                <groupId>org.codehaus.mojo</groupId>
                <artifactId>build-helper-maven-plugin</artifactId>
                <inherited>true</inherited>
            </plugin>

the LocalizationMessages would be generated in this new module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The container shares the original package because most of the functionality is reused from the org.glassfish.jersey.jetty.JettyHttpContainer however, the given class has package private constructors which makes impossible to initialize it outside of the package. I did not want to modify the original container, so I've just shared the original package. However, with JPMS this won't work. What do you advise as a proper solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

The best looks extending JettyHttpContainerProvider by public <T> T createContainer(final Class<T> type, final Application application, Object parentContext)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, will try

@jansupol
Copy link
Contributor

jansupol commented Apr 4, 2023

Please update bom.pom

Signed-off-by: Maxim Nesen <maxim.nesen@oracle.com>
@senivam senivam merged commit dc74036 into eclipse-ee4j:master May 2, 2023
@senivam senivam deleted the 2x_http2_jetty_container branch May 2, 2023 08:21
@senivam senivam added this to the 2.40 milestone May 3, 2023
srowen pushed a commit to apache/spark that referenced this pull request Oct 29, 2023
### What changes were proposed in this pull request?
This pr aims upgrade Jersey from 2.40 to 2.41.

### Why are the changes needed?
The new version bring some improvements, like:
- eclipse-ee4j/jersey#5350
- eclipse-ee4j/jersey#5365
- eclipse-ee4j/jersey#5436
- eclipse-ee4j/jersey#5296

and some bug fix, like:
- eclipse-ee4j/jersey#5359
- eclipse-ee4j/jersey#5405
- eclipse-ee4j/jersey#5423
- eclipse-ee4j/jersey#5435
- eclipse-ee4j/jersey#5445

The full release notes as follows:
- https://github.com/eclipse-ee4j/jersey/releases/tag/2.41

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GitHub Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #43490 from LuciferYang/SPARK-45636.

Lead-authored-by: YangJie <yangjie01@baidu.com>
Co-authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
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.

3 participants