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

Support Jetty 11 #162

Closed
f4lco opened this issue May 9, 2020 · 8 comments
Closed

Support Jetty 11 #162

f4lco opened this issue May 9, 2020 · 8 comments

Comments

@f4lco
Copy link
Collaborator

f4lco commented May 9, 2020

After #159 laid out groundwork for Jakarta EE, we require to add support for Jetty 10.

Implementation-wise some groundwork still exists (such as properties for Jetty dependency versions etc). Disabled switches have a comment with this issue number: #162. It is probably best to re-instantiate "gretty-runner-jetty94" from master, rename it to "*-jetty10", pull in new dependencies, and to enable / run the tests.

This issue should set the default servlet container back to Jetty, and also set the default scanner manager back to Jetty. The default container was previously set to Tomcat, and the scanner to JDK, because Tomcat was the only Jakarta-enabled servlet container at the time. To be compatible with earlier releases of Gretty, the defaults should be same as for prior releases of Gretty, given they are implemented.

@boris-petrov
Copy link
Member

I saw @f4lco working on a Jetty 11 branch (thanks for that!) and wondered what happened with Jetty 10... 😄 So actually Jetty's story is a bit strange:

jetty/jetty.project#4578 (comment)

If that comment is what will indeed happen, then perhaps Gretty 4 should support Jetty 11+ while Gretty 3 could support Jetty 10.

@f4lco
Copy link
Collaborator Author

f4lco commented Nov 10, 2020

@boris-petrov yes, I dug into it a bit, but my timebox ran out :/ Right now I don't know when it's gonna land, there are several blockers. logback requires update and dropped GafferConfiguration (temporarily, they say - that's more than one year ago), and currently the single-sign-on feature is broken, which appears difficult to fix.

then perhaps Gretty 4 should support Jetty 11+ while Gretty 3 could support Jetty 10.

IMO that's a good idea.

@boris-petrov
Copy link
Member

@f4lco thanks for the work you put into everything!

Let me ask about these blockers:

  1. For the logback update you mean this commit?

  2. Not sure what this GafferConfiguration is - some class somewhere?

  3. single-sign-on is broken where - in Gretty or in Jetty or they changed something in Jetty 11 that will require work in Gretty to be fixed?

@f4lco
Copy link
Collaborator Author

f4lco commented Nov 10, 2020

  1. No, concerning logback it's it's ea09eae on the feature branch. I think Jetty 11 is supposed to work with slf4j 2.x, which only logback 1.3 is compatible with, which dropped support for configuration via Groovy scripts (as of time of writing).
  2. GafferConfiguration: fancy name for 'configure logback using Groovy scripts'. Inside Gretty, we have GafferConfiguratorEx class which implements the functionality behind Grettys logging toggles.
  3. Something appears to have changed in Jetty which will require rework in Gretty. I currently end up with an NPE, and the root cause is as follows: apparently the SessionHandler cannot be shared safely between different contexts (any more, reference: JettyConfigurerImpl#configureSessionManager). When sharing the SessionHandler, startup of the first webapp branches into the lifecycle of the second webapp via this shared handler, but the second webapp is not initialized yet.

@boris-petrov
Copy link
Member

The commit that I linked downgrades Logback to 1.1.x from 1.2.x because updating it caused the tests to fail. It would be strange for 1.3.x to work out-of-the-box - perhaps first version 1.2.x should be used.

You are correct that Jetty 11 supports only slf4j 2.x. And yes, as of 1.3.0-alpha4 Logback doesn't support Groovy. But they say it's because currently available versions of Groovy are incompatible with Java 9 which hasn't been true for years now so hopefully they will fix that and we won't have to do anything in Gretty.

As for number 3 I have no idea what you're talking about. 😄 The same is not a problem with Tomcat 10?

@f4lco
Copy link
Collaborator Author

f4lco commented Nov 13, 2020

As for number 3 I have no idea what you're talking about. 😄 The same is not a problem with Tomcat 10?

No, it's not related to Tomcat, this SessionHandler thing is a Jetty-specific concept. Nvm, it looks like I figured out an alternative approach, I'm running the tests right now.

@boris-petrov boris-petrov changed the title Support Jetty 10 Support Jetty 11 Nov 17, 2020
@boris-petrov
Copy link
Member

I think this issue can be closed now, @f4lco implemented Jetty 11 support in the gretty4 branch! 🎉

@f4lco
Copy link
Collaborator Author

f4lco commented Nov 19, 2020

@boris-petrov I even wrote an reminder in this ticket, but totally forgot! 🙈

This issue should set the default servlet container back to Jetty, and also set the default scanner manager back to Jetty.

I already addressed this concern in c45f73c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants