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

[OPENJPA-2762] Update JMS spec to v2.0 and set scope to 'provided' #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattrpav
Copy link

No description provided.

@rmannibucau
Copy link
Contributor

due to activemq (the real one, the v5 ;)) being widespread, we should stick to jms 1 which will ensure we have the right osgi metadata for amq5 and jms 2 impls IMHO

@mattrpav
Copy link
Author

@rmannibucau haha =). Good news though, by bumping to JMS v2.0 it won't break ActiveMQ 5.x compatibility. Rather, the JMS code in OpenJPA only uses the JMS v1.1 APIs and the JMS v2.0 API is backwards comparable. Also, ActiveMQ 5.x is adding JMS v2.0 support starting in 5.17.0, so updating this will be a good thing for ActiveMQ 5.x support going forward =).

Not to mention, eventually we'll have to bump OpenJPA to use the jakarta.jms (v3.0) jars coming soon.

@rmannibucau
Copy link
Contributor

@mattrpav well AMQ being so much widespread, 5.17 is not sufficient to upragde ;). Al note that your assumption JMS 2 is backward compatible is wrong in terms of OSGi metadata (osgi.contract) so we must stick on JMS 1 to ensure we run with JMS 1 and JMS 2. For jakarta, and due to our JMS usage (and future usage), I think we'll just do as several other asf project and use maven shade plugin to provide a jakarta friendly packaging without breaking javax which stays mainstream for now nor forking the code.

@mattrpav
Copy link
Author

@rmannibucau what do you mean by "wrong in terms of OSGi metadata"? There are only two classes in OpenJPA that make use of JMS, and they both use the API signatures that are compatible with both JMS 1.1 and 2.0.

If we do not change the OSGi import, OpenJPA users would not be able to use current versions of IBM MQ or Tibco EMS either.

I believe the OSGi import in this PR is correct, saying "OpenJPA will support using v1.1 up to (but not including 3.0)". This is the same OSGi import setting that other Apache projects have also adopted.

ref: https://github.com/apache/openjpa/search?q=javax.jms

@rmannibucau
Copy link
Contributor

@mattrpav I'm not speaking of code (so don't look in .java) but of MANIFEST.MF data. We have an osgi.contract requiring JMS 1 normally. It means we can work with JMS 1 and 2. What is wrong seems the pom - but your PR does not change anything at that level. Typically:

 javax.jms.*;version="[1.1.0,1.2)"

should now be

 javax.jms.*;version="[1.1.0,2.1)"

(tip: for jms 3 it should be rewritten with the shade plugin transformer).

We should probably also ensure we add JMS as an osgi.contract requirement I think but not in a hurry on this one.
So overall I am -1 to use jms-2 since it will break as explained, +1 to enable OSGi import to range to v2.1 to enable JMS 2 usage in OSGi.

Hope it makes sense.

@mattrpav
Copy link
Author

@rmannibucau yep, I'm aware of the osgi manifest declarations. The maven bundle plugin generates those values based on the maven dependencies.

Again, by staying with the JMS 1.1 requirement, we are limiting JMS providers, and making it more difficult to integrate OpenJPA with other projects. For example, recent versions of Camel ship with JMS v2.0 jar dependencies. If we force OpenJPA to stay with 1.1, we will never get a valid wiring for a project that depends on Camel+OpenJPA.

@rmannibucau
Copy link
Contributor

Again, by staying with the JMS 1.1 requirement, we are limiting JMS providers, and making it more difficult to integrate OpenJPA with other projects. For example, recent versions of Camel ship with JMS v2.0 jar dependencies. If we force OpenJPA to stay with 1.1, we will never get a valid wiring for a project that depends on Camel+OpenJPA.

This is wrong, osgi knows that v1 works with v2, this is how all specs are done btw.
The issue is the range as mzntionned which is not the dependency but explicit manual value as writtzn previously.
Once again, i agree we should fix the range but we shouldnt upgrade the dependency as explained.

@mattrpav
Copy link
Author

mattrpav commented Oct 3, 2020

@rmannibucau I'm not following you at all. When would you think it is suitable to upgrade to the v2 of the JMS specification?

Note-- in the patch, I set the dependency as provided to allow for users to determine at runtime.

@rmannibucau
Copy link
Contributor

@mattrpav when we would use jms2 api only (not tomorrow IMHO ;)). Once again, issue is not about the runtime but manifest metadata + proposed api whzn developping the project. If your goal is to run on jms 2 with osgi, please fix the pom as mentionned, no need of a dependency change. If you want to run in a EE container or standalone app, we already do it.

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.

2 participants