-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-14910 NIFI-14911 NiFi - Upgrade Jetty to 12.1.1 and move to Jakarta EE11 #10249
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough work on these changes @pvillard31. The major of the adjustments appear to be straightforward.
Aside from the thread pool handling, the only code-related adjusted that raised questions is in JettyServer
, with the associated changes to add the jetty-deploy
dependency.
Regarding Spring Boot and NiFi Registry, with the way the application is structured, it seems like it should be possible to use 12.1 and override some references. Is the problem there related to Jakarta EE 11 as opposed to Jetty 12.1 itself? It is fine to address that separately, but it would be helpful to have more details.
...nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
Outdated
Show resolved
Hide resolved
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.jetty</groupId> | ||
<artifactId>jetty-deploy</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jetty-deploy
library brings in several other dependencies. Is there a reason this is necessary in Jetty 12.1 and not 12.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is one of the changes with Jetty 12.1: DeploymentManager
no longer exists and I'm replacing it by StandardDeployer
which comes from jetty-deploy
in JettyServer
...ork/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/StandardServerProvider.java
Outdated
Show resolved
Hide resolved
...-test-suite/src/test/java/org/apache/nifi/tests/system/clustering/FlowSynchronizationIT.java
Show resolved
Hide resolved
For NiFi Registry, reviewing further, the problem is specifically related to EE11. I have made the changes to also uses Jetty 12.1.1 in NiFi Registry but keep the ee10 layer. |
OK... so while using Jetty 12.1.1 works for NiFi Registry (I was able to properly build it, run it, and use it with NiFi), the integration tests are failing because of the way Spring Boot is used there (I remember now :)). And we can also see the failure on the system test that I was mentioning with the changes on |
Thanks @pvillard31. As far as the NiFI Registry integration tests, I am open to removing those because they do not follow the actual pattern of a running NiFi Registry. The NiFi Registry Docker tests also provide some basic checks for a running NiFi Registry. What do you think? On the system test failure, it is a bit strange to see the change, but if the previous adjustments corrected it and were otherwise reasonable, that sounds good. |
Yeah so for NiFi Registry, I noticed that we were bringing Jetty through Spring Boot for running the tests. I removed this import to instead used the embedded Tomcat to still execute those tests successfully. I'm not against completely removing those. |
That looks like a reasonable and minimal change to keep those tests passing. If that works and doesn't impact runtime behavior for NiFi Registry, that sounds good. |
…rta EE11 Signed-off-by: Pierre Villard <[email protected]>
… to avoid conflict ; restore changes to avoid test failure in system tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extensive work here @pvillard31.
On final validation, I noticed an exception thrown from LogoutCompleteRedirectFilter
related to the Fragment
portion of the redirect URI when passed to sendRedirect()
. Changing the approach to set the Location
header and send an HTTP 302 as in QueryStringToFragmentFilter
worked as expected, so I will include that change in the merge commit.
Summary
NIFI-14910 NIFI-14911 NIFI-14913 NiFi - Upgrade Jetty to 12.1.1 and move to Jakarta EE11
Important notes:
4.0.0-M4
. No clarity as to when the final4.0.0
release is expected.javax.jms-api
tojakarta.jms-api
as this would be a breaking change for users with existing JMS components where a specific JMS client is provided as part of the configuration. This is something we can evaluate later.Testing:
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation