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

Enable use of Tomcat 10, a Jakarta-enabled servlet container #159

Merged
merged 8 commits into from
May 22, 2020
Merged

Enable use of Tomcat 10, a Jakarta-enabled servlet container #159

merged 8 commits into from
May 22, 2020

Conversation

f4lco
Copy link
Collaborator

@f4lco f4lco commented May 6, 2020 •
edited
Loading

This PR represents the result of the discussion in #144. I also had to make some organisational changes which may require adjustment (or reversal):

  • Because these are breaking changes, I set the Gretty version to the next major version, 4.0.0-SNAPSHOT.
  • Therefore this PR targets a new "gretty4" branch, in which all Jakarta servlet containers can stabilise. A couple of (very important) pieces are still missing, which are outlined below.
  • Because "Jakarta-Gretty" only supports Tomcat right now, it is also the default servlet container if none is specified. Probably Jetty should be default again once supported.

Changes

  • Removed support for all non-jakarta servlet containers
  • Rewritten SSL support for Tomcat (former used deprecated and now removed properties)
  • Minor fixes: httpsIdleTimeout, maxPostSize

Missing pieces

Especially the minimum JDK version would require further discussion. With the state in this PR, Gretty requires all users to use Java 11 at a minimum, although Tomcat 10 only requires 8+. I can only guess how many users would be impacted.
If one were to change that, there are two challenges: it requires refactoring, because the "gretty" module can no longer depend on "jetty-util", and changes in packaging, such that there are two Gretty variants, one of which supports only Tomcat and JDK 8+, and another, which ships Tomcat and Jetty connectors and supports JDK 11+. I would leave that discussion for a new issue.

If you think it's a good idea, I would create issues for each bullet point of the missing pieces. What do you think about creating a milestone for that?

@f4lco f4lco requested a review from boris-petrov May 6, 2020 08:29
@boris-petrov
Copy link
Member

Brilliant work, thank you so much for your time!

I have a few general questions. I'll also add some comments inline later.

  1. Are you feeling confident enough with Jetty to implement support for it or that's something you're not willing/have the time to do?

  2. The SSL changes for Tomcat (I believe this commit) - are the new properties used working in Tomcat 9 too? That is, can't this commit be ported to master right now?

  3. The same for http(s)IdleTimeout - can't we migrate this directly to master? This will fix a real problem in the current Gretty and decrease the size of this PR (which is always good).

  4. About maxPostSize. In the commit you've written "compare:" but given only one link. I believe this is mistyped because the documentation for Tomcat 9 states the same, right? If that's so, again, can this commit go to master?

  5. Same for the assert commit? Great idea there, by the way!

  6. I'm not sure I understand the problem with Jersey. There is a new major version but inside it they still use the old imports? Am I understanding correctly? Why would they do that?

  7. Is there an open issue for the Spring stuff? So you can add a TODO with a link in the commit.

  8. As for the JDK requirements change. I personally don't care because I'm using the latest JDK but I guess most people are still stuck on 8 so there probably will be shouting. You're more well-versed in the code right now - how much is the gretty module dependent on jetty-util? Is it going to be a lot of work to not use it? If so, I guess none of us would want to spend time on it (unless you're also one of the people that require JDK 8 support) and we can just leave it requiring JDK 11. As for the packaging part - I'm not sure you're right there. If Gretty doesn't use anything from Jetty at build-time (so JDK 11 is not required at build-time) I guess it would work on JDK 8 too (because Jetty 10 would be a runtime-only thing so someone using Tomcat 10 won't need JDK 11). I'm not sure I'm correct in this though.

  9. I'm fine with creating a milestone and issues for the bullets. I'm not that familiar with GitHub's workflows so I'll follow whatever you suggest. :)

Thanks again for the great work!

@boris-petrov boris-petrov marked this pull request as draft May 7, 2020 07:35
@boris-petrov
Copy link
Member

The README must be updated (the first paragraph) to note the supported Tomcat and Jetty versions.

@f4lco
Copy link
Collaborator Author

f4lco commented May 8, 2020 •
edited
Loading

Wow, that's much to unpack! Thank you very much for your thorough review.
Then let's start with remarks 2 through 5 in batch. You're completely right, those commits are indeed good candidates for master, and I'll test their compatibility and shoot another PR your way.
All other remarks are valid, too. I'll answer them one by one after that first step is complete.

@f4lco
Copy link
Collaborator Author

f4lco commented May 8, 2020

Are you feeling confident enough with Jetty to implement support for it or that's something you're not willing/have the time to do?

In principle that is something I could implement. I guess my primary constraint is time. While I have (obviously) found some enjoyment in hacking Gretty, all the "missing pieces" are tasks are free to be picked up by anybody who is willing to put time into it.

@f4lco f4lco mentioned this pull request May 8, 2020
Merged
@f4lco f4lco force-pushed the feature/jakarta branch from 5822af0 to 0cd56c2 Compare May 9, 2020 06:24
@f4lco f4lco mentioned this pull request May 9, 2020
Closed
@f4lco
Copy link
Collaborator Author

f4lco commented May 9, 2020

I'm not sure I understand the problem with Jersey. There is a new major version but inside it they still use the old imports? Am I understanding correctly? Why would they do that?

To my understanding that is correct. I assume that the API artifact "jakarta.ws.rs:jakarta.ws.rs-api" was comparatively trivial to rename and thus is available earlier than the core functionality. I think the Jersey project is simply halfway in the Jakarta migration.

Is there an open issue for the Spring stuff? So you can add a TODO with a link in the commit.

Which project should contain the open issue? As for Spring (Boot) I cannot identify a singular issue or milestone I could link to. I'll create a ticket in Gretty as already promised. I can add that reference to the commit, no problem.

As for the JDK requirements change. [...]

I extracted this discussion to #161 and already left a reply there. As far as I know there is build-time dependency on JDK 11, which is bad news, because it requires more effort.

I'm fine with creating a milestone and issues for the bullets. I'm not that familiar with GitHub's workflows so I'll follow whatever you suggest. :)

Neither am I, but I think we do need some overview of what's missing. Also when playing the "waiting game" for releases of other projects. I hope that a milestone exactly does that.

@f4lco
Copy link
Collaborator Author

f4lco commented May 9, 2020

Hopefully I addressed all of your concerns. I left two fixup commits such that you can immediately see how that was done. I'll do the organisational stuff (creating tickets, milestone) later.

@f4lco f4lco marked this pull request as ready for review May 9, 2020 08:07
This was referenced May 9, 2020
Closed
Closed
Open
@boris-petrov
Copy link
Member

Great, all seems good, thanks again! The only two questions remaining for me are:

  1. Why isn't there an issue in Spring (Boot) for the changes they have to make? Either they don't care about the new stuff coming or it's not coming soon. In any case, we'll have to keep looking from time to time.

  2. About the minimum JDK requirements. I'll write about that in the issue.

@f4lco
Copy link
Collaborator Author

f4lco commented May 18, 2020

Sorry for the longer response time than usual... to me last week was.... rough.

Why isn't there an issue in Spring (Boot) for the changes they have to make? Either they don't care about the new stuff coming or it's not coming soon.

I dug deeper and (already completed) Spring Boot milestone 2.3.0.M3 (https://github.com/spring-projects/spring-boot/milestone/155?closed=1) contains several dependency upgrades hinting at Jakarta: spring-projects/spring-boot#20510, spring-projects/spring-boot#20452, spring-projects/spring-boot#20453, spring-projects/spring-boot#20451. I hope that important artifacts such as "jakarta-servlet-api" follow swift.

That's what I wanted to describe with "singular issue": I failed to pinpoint a single issue / milestone that says "Spring Boot is now ready for Jakarta", which we can simply watch. Perhaps Gretty is in a difficult role, because it is sandwiched in between servlet containers and application frameworks. I do think that trademark will be removed in the long run, even if it's a bit early to tell when major framework releases are out of the door.

I'll rebase this branch and squash fixups as appropriate.
Thank you so much for the commitment for doing this with me 😀

f4lco added 8 commits May 18, 2020 14:50
@f4lco
Remove all non-jakarta servlet containers (#144)
dfa4142
@f4lco
Remove all references to non-jakarta servlet containers from integrat…
5dec04b 
…ion tests (#144)
@f4lco
Rewrite all imports to jakarta in integration tests (#144)
a52f5c5
@f4lco
Adjust TomcatServerConfigurer to Tomcat 10 API (#144)
c1e7e89 
The WebappLoader lost the ctor with the parent class
loader parameter and instead sets the reference from
the started context.
Alas, it should be possible to directly set the parent
class loader on the context, and it should just function
as before.
@f4lco
Upgrade Jersey and disable test for now (#144)
31890f1 
While a recent Jersey version provides "jakarta"-ified APIs,
we are still unable to deploy it because Jersey depends on
the javax.servlet classes internally (for instance in
org.glassfish.jersey.servlet.ServletContainer).
We'll have to wait for a compatible release of Jersey.
@f4lco
Disable Spring integration tests due to lack of support for Jakarta (#…
ccdaaa2 
…144)
@f4lco
Drop JDK 8 builds due to Jetty 10 requirement (#144)
86e9158 
There is already some preliminary work which pulls in
dependencies for Jetty 10. Because Jetty 10 raises the
minimum supported Java version to 11, Gretty will
also require JDK 11 to compile and run.
Source: https://www.eclipse.org/jetty/documentation/current/what-jetty-version.html
@f4lco
Remove obsolete methods TomcatConfigurer#setEngine and TomcatConfigur…
c865692 
…er#setService (#144)
@f4lco f4lco force-pushed the feature/jakarta branch from cf289d0 to c865692 Compare May 18, 2020 12:51
@boris-petrov
Copy link
Member

Good work finding these issues! So they will be supporting the new APIs sooner or later. We'll wait and see.

By the way, I'm not sure whether you want to leave the PR open until everything is resolved or merge it in the branch and then add new commits? I'm fine either way.

As for my help - you're welcome! You're doing 99% of the work so the thanks goes to you! :) Beers are on me! :)

@f4lco
Copy link
Collaborator Author

f4lco commented May 18, 2020

I guess it's good to have that one (possibly long-lived) branch "gretty4" and regularly merge into it than to have a stale PR. After all, this PR completes issue #144, which then can be closed. For the remainders of the milestone the devs can create new feature branches, which will once more target "gretty4".

@f4lco
Copy link
Collaborator Author

f4lco commented May 22, 2020

Can I conclude this PR is good to go? Did you finish your review, does the end result look good to you? Wow, that's 6k in removed lines btw 😲

@boris-petrov
Copy link
Member

Yes, I think the PR is great! Go ahead and merge. Removing tons of code is my favorite thing. 😄

@f4lco f4lco merged commit c865692 into gretty4 May 22, 2020
@f4lco f4lco deleted the feature/jakarta branch May 22, 2020 14:27
@f4lco f4lco linked an issue May 22, 2020 that may be closed by this pull request
Support Apache Tomcat 10 #144
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boris-petrov boris-petrov boris-petrov left review comments

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

Successfully merging this pull request may close these issues.

Support Apache Tomcat 10
2 participants
@f4lco @boris-petrov