Repository navigation
Update JmsIO to ActiveMQ 6.2.5 and jakarta.jms - #38729
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the JmsIO module by migrating from the legacy javax.jms namespace to the Jakarta Messaging API (jakarta.jms). This change involves updating core dependencies to versions that support the Jakarta EE standard, ensuring compatibility with modern messaging infrastructure. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request migrates the JMS IO module from the legacy javax.jms namespace to the modern jakarta.jms namespace, upgrading ActiveMQ to 6.2.5, Qpid JMS Client to 2.10.0, and adopting jakarta.jms-api:3.1.0. The review feedback identifies critical Java compatibility issues with these upgrades: ActiveMQ 6.x requires Java 17, while Qpid JMS Client 2.x and Jakarta JMS API 3.1.0 require Java 11, which would break Apache Beam's Java 8 and Java 11 compatibility. The reviewer suggests downgrading the Jakarta JMS API to 3.0.0 to maintain Java 8 support and requests verification on how compatibility will be maintained for the other dependencies.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
52fe765 to
dc7ebca
Compare
a1f3ae2 to
a85672c
Compare
|
Assigning reviewers: R: @ahmedabu98 for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| // map. Because the global resolution strategy forces every mapped version, | ||
| // that 6.2.5 would otherwise be forced onto this Java 11 module via the shared | ||
| // org.apache.activemq coordinates. MQTT IO only needs ActiveMQ as an embedded | ||
| // test broker, so pin it back to the Java 11-compatible 5.x line. |
There was a problem hiding this comment.
I see a couple of mentions of making this compatible with Java 11, but then we're forcing Java 17 - do we actually need to force Java 17 or can we just require Java 11?
There was a problem hiding this comment.
ActiveMQ is a test dependency for AmqpIO, MqttIO, JmsIO. Instead of modifying build.gradle in the former ones, can we just pin to ActiveMQ 6 in JmsIO? This should give a smaller diff and safer as it doesn't do a global upgrade.
There was a problem hiding this comment.
PS: I'm thinking about we should move our Infra to Java17 (probably Java21, to reduce the frequency of this kind of upgrade) while keep Java11 compatibility via cross-compilation. We already use Java21 to publish Javadoc
|
@damccorm the goal is to support the latest JMS spec version and so latest ActiveMQ version. |
|
Sounds good - I'm good with the change pending the one open comment thread |
|
Also, JmsIO used to have some weird checkpointing behavior to accommondate Jms spec
This caused data loss due to Beam process and commit messages asynchronously. We ended up creating different sessions for each checkpointing: #30054 SInce this is a Jms spec upgrade, let me check if any breaking changes could happen especially checkpointing logics |
|
Reminder, please take a look at this pr: @ahmedabu98 @damccorm @kennknowles |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @chamikaramj for label java. Available commands:
|
|
Updates: there will be significant changes in JmsIO (support ACKNOWLEDGE_MODE and Python xlang support) in next release. Current plan is to have one (or two) stable version before we moving to Java17 |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
@damccorm I will rebase. I think this PR is good to go after rebase. |
|
Hi @jbonofre, will you be rebasing soon? thanks |
a85672c to
e7961f1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #38729 +/- ##
============================================
+ Coverage 56.16% 58.75% +2.58%
- Complexity 2288 13918 +11630
============================================
Files 1124 2585 +1461
Lines 178336 271699 +93363
Branches 1489 11243 +9754
============================================
+ Hits 100170 159636 +59466
- Misses 75645 106060 +30415
- Partials 2521 6003 +3482
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| - name: Setup environment | ||
| uses: ./.github/actions/setup-environment-action | ||
| with: | ||
| java-version: '17' |
There was a problem hiding this comment.
We no longer need these lines as the CI is on Java 21 currently
| activemq_junit : "org.apache.activemq.tooling:activemq-junit:$activemq_version", | ||
| activemq_kahadb_store : "org.apache.activemq:activemq-kahadb-store:$activemq_version", | ||
| activemq_mqtt : "org.apache.activemq:activemq-mqtt:$activemq_version", | ||
| activemq6_amqp : "org.apache.activemq:activemq-amqp:$activemq6_version", |
There was a problem hiding this comment.
if activemq 5 and 6 have same package name, BeamModulePlugin version resolution doesn't work well as it tries to enforce a version it first see. As you may have already noticed and added force resolution in io.amqp
Consider simply bump activemq deps to v6 and pin to v5 in activemq, which has same effect
Bump activemq to 6.2.5 and qpid-jms-client to 2.10.0, both of which target jakarta.jms. Replace the geronimo-jms_2.0_spec dependency with jakarta.jms-api 3.1.0 and migrate all javax.jms imports and javadoc references in JmsIO main and test sources to jakarta.jms.
3792538 to
df1695b
Compare
Migrate JmsIO to jakarta.jms (JMS 3.1) with ActiveMQ 6.2.5 and qpid-jms-client 2.10.0. Replaces geronimo-jms_2.0_spec with jakarta.jms-api 3.1.0 and renames all javax.jms imports to jakarta.jms in main and test sources.
JmsIO now requires Java 17. AMQP and MQTT modules are unaffected (kept on ActiveMQ 5.19.2 via a separate activemq6_* library entry).