[AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3 #682
Conversation
|
As discussed on the mailing list, I did it already and I was about to create the PR. |
|
By the way, I'm using Geronimo spec to be aligned with camel and OSGi. |
|
@jbonofre this change is purely packaging skeleton. I know you mentioned working on full JMS 2.0 support along w/ big things like replicated-kahadb. I thought I'd pitch in to help get the ball rolling on the low-hanging fruit. There is no real code change here, so I think this PR good to merge and should slide in with your real JMS 2.0 impl work. I also broke up the 2.0 Impl work into sub-tasks here-- https://issues.apache.org/jira/browse/AMQ-7309. This should help parallelize the effort amongst contributors. Reviewing other projects and frameworks, I agree the geronimo vs jakarta is the real question here and I think it makes more sense to go to the Jakarta jar now as it is more target state than using geronimo. If we go geronimo users will have this to look forward to: Alternatively, if we go jakarta now, users have this: CXF has transitioned many spec jars to jakarta, and I suspect Camel will do the same as well. I think the alignment makes more sense for CXF and Camel to consume the dep ActiveMQ (being as ActiveMQ is the JMS provider as those ride on top of ActiveMQ), or they simply provide the . Either way, the JEE transition is going to force framework devs and users into being savvy with Maven and I do not think there is any way around that. I'd be interested in hearing others' perspective as well. |
activemq-client/src/main/java/org/apache/activemq/ActiveMQConnection.java
Outdated
Show resolved
Hide resolved
| */ | ||
| @Override | ||
| public JMSContext createContext(String userName, String password) { | ||
| throw new UnsupportedOperationException("createContext() is not supported"); |
There was a problem hiding this comment.
I like the way you're calling out the specific methods in the exceptions. Makes it clear to diagnose. But you should standardize on having "createContext(String, String)" or just "createContext()", and same with the other unimplemented methods.
There was a problem hiding this comment.
Just missed this one.. I'll fix in the follow-up commit this morning
| @@ -142,8 +142,8 @@ | |||
| <dependencies> | |||
| <!-- j2ee jars --> | |||
| <dependency> | |||
| <groupId>org.apache.geronimo.specs</groupId> | |||
| <artifactId>geronimo-jms_1.1_spec</artifactId> | |||
There was a problem hiding this comment.
@jbonofre what's the advantage of using geronimo vs. jakarta? I know you mentioned in your implementation you used geronimo - why is one more likely to create conflicts with Camel?
There was a problem hiding this comment.
Osgi support, will likely not hit jakarta anytime soon whereas G does/will maintain it so best choice stays G as of today.
There was a problem hiding this comment.
That's my point, agree with Romain: it's my proposal, don't use Jakarta but rather Geronimo Spec instead. It's what other projects are doing (especially Camel). Geronimo Spec supports OSGi cleanly, so, it's better to use it instead of Jakarta.
There was a problem hiding this comment.
I have some more changes coming in this morning.
There was a problem hiding this comment.
i see that this PR is still using jakarta instead of geronimo , was the mentioned osgi compatibility resolved?, whats the story/consensus, from above it seems that it consensus was to use geronimo but PR isnt updated from that.
@mattrpav for a usability perspective, whatever approach we take, I think just being helpful on the website is sufficient -> have a page that clearly states what is supported for JMS, what isn't, what would be the required/removed dependencies for maven, gradle and some concise examples |
|
@jbonofre @rmannibucau I've updated this round of the PR to include geronimo-jms for the karaf artifacts. While there is no bigger Karaf fan than me, I think using jakarta as the baseline makes more sense. Please here me out on these points below and if you still think we should go geronimo all the way, I'll make the change. Most users are going to see activemq through the lens of activemq-client and its dependencies: vs Shortly, we'll have:
Thank you for hearing me out |
|
Up vote for jakarta.jms-api-2.0.3 |
|
Up vote for geronimo-jms_2.0_spec-1.0-alpha-2 |
@tabish121 agreed. The intent here isn't to 'sneak' it in. A workable solution didn't present itself while making the API change. Before anything is merged, I'll separate out the changes. |
So overall if you want OSGi you have to use geronimo, if not it is asf vs eclipse only. |
|
"Either way, the JEE transition is going to force framework devs and users into being savvy with Maven and I do not think there is any way around that." Its really not that dissimilar to the situation where where people could use any of the many particular 2.0 API jars they desire in their applications already, even with a 1.1 impl if they want to, prior to this change (which I dont think its a good idea personally). |
|
Note that by specifically adding an implementation of setJMSDeliveryTime that throws UOE, this change will likely actually break sending of ActiveMQ 5.x messages through other existing JMS 2 providers that handle being given JMS 1.x message objects not implementing the method, as they wont handle it being implemented but deliberately throwing UOE. Unlike the session etc they cant check any version metadata first to avoid the method as all they see is the message. |
@gemmellr That is an extreme edge use case-- one that is acceptable for a RC release. These JMS 2.0 methods won't be unimplemented for long. Feel free to add a documentation note if you know of some users that have this scenario and would be impacted. |
@rmannibucau I know what Geronimo is ;-). My point was to consider the perspective of new-to-Java and new-to-JMS users that are using ActiveMQ for the first time. The latest version of the PR provides gerinomo for OSGi/Karaf users (myself included) and has a the Jakarta jar for non-OSGi use cases. |
Based on how slowly things can happen sometimes I would push back against saying anything like "these methods won't be unimplemented for long" as who knows if/when it will be finished. @gemmellr - Can you describe this scenario in more detail or a specific use case? I want to make sure we are not breaking anything with throwing unimplemented exceptions and how big of an edge case it is. |
I'm not sure I'd agree that bridging between different systems was 'extreme-edge'. Not the primary use, sure, but hardly strange and unusual either. Not sure I have seen mention of doing 'RC releases' before. Are these to be named 5.17.0-rc1, rc2 etc given it is targeting main, or is it for after that as a series of 5.18.0-rcX? Might seem odd to use 5.17.0 given that should probably be out and done already (and would then be looking for 5.17.1 etc for bug fixes), while this is unimplemented. Given that it even seems a stretch calling it an -rc, to me that usually implies near-completion. |
Take an ActiveMQ 5 JMS message object, and send that JMS Message somewhere else using a different JMS client. E.g receive from a 5.x broker and publish using the same message object to another type of broker...could be due to communicating with different teams, companies, or handling a system transition, etc. As part of that process if the other provider is actually a JMS 2 client, it will try to stamp the JMSDeliveryTime on the original message object like its meant to. It may have handled the method not being implemented at all on JMS 1.x message objects since thats not so unexpected due to spec transition, but will likely fail if the method actually exists but simply throws UOE as thats unexpected. Basically im saying at least implement the setter method and make 5.x call it to set it to 0 during send (also handling the reverse case of sending other messages through 5.x, ensuring they are properly stamped). |
@gemmellr yeah, so tricky corner of the JMS API. This trips up a lot of folks that are new to JMS-- the setDeliveryDelay can only be honored when set on the MessageProducer object. Any code that calls it on the Message is not valid per the spec. For the JMS bridge idea, I can't walk a scenario where would be actually problematic. This feels hypothetical as it has a chicken-and-egg problem. The JMS API doesn't define how message data is hydrated into the Message object. It is handled by the provider impl and the wire protocol-- there is not support for the DeliveryDelay field in OpenWire at this time and there is nothing in ActiveMQ code base that calls setDeliveryDelay, and then throw an exception unexpectedly on a user. As far as anyone providing a JMS 2.0 wrapper for ActiveMQ-- any code that is currently calling ActiveMQ using a JMS 2.0 MessageProducer to set a DeliveryDelay must have a custom wrapper impl for MessageProducer. Those scenarios are out-of-reach edge cases. |
@cshannon that's fair. Check out the approach notes on the impl approach on the DeliveryDelay specific sub-task: https://issues.apache.org/jira/browse/AMQ-8320 Basically, I think we can do a first-pass without OpenWire changes, and then tee-up a OpenWire v13 ticket to group up enhancements and minimize the protocol impact. |
|
@mattrpav I'm not actually all that new to JMS at this stage really, I'm at least a little aware what the methods are for and how the functionality is used, having implemented JMS [2] in full including a full protocol mapping of the functionality in a client capable of doing such scheduled delivery against ActiveMQ 5 for several years now already. I have no idea why you think I was talking about application or 'wrapper' usage of the setter, I wasn't. I think I was pretty clear about taking a 5.x message object and sending it through another JMS 2 client, which is where the issue arises, as it necessarily tries to call setJMSDeliveryTime on every message sent (as either there is a delay, and the actual time needs set, or there isnt and it should be clearing the potentially-non-zero existing value to indicate so). Such a client client may have reasonably handled a JMS 1.1 object not having the method implemented, as e.g. 5.x didnt so far, but it is not going to expect the method to be actually implemented but throw UOE. These methods exist precisely for this use case, for [foreign] providers to set the needed values on message objects being sent [which arent their own implementation].
If 5.x is going to use the JMS 2 API then any JMS 2 message object sent with it should reasonably be expected to come back with a delivery time value of 0 after send if there is no delay occurring, i.e it should be doing the equivalent of setJMSDeliveryTime(0) by itself on its own messages, and actually calling setJMSDeliveryTime(0) on other providers messages to clear any existing non-zero value. |
|
@gemmellr replies inline..
Cool
Yeah, I'm trying to piece your scenario together, but coming up short. Am I getting this right:
I don't see how #4 happens with #3 is in place as you indicated. I see a chicken-and-the-egg problem. As as side-effect, by swallowing the 'set' and returning '0' on get, we'd be silently wiping out the DeliveryDelay and users would have no indication as to why DeliveryDelay is not occurring. It is just as reasonable to argue this should error out so it is caught in unit/integration tests before they roll to prod.
I think it is an interesting suggestion-- there is a JIRA open for the DeliveryDelay impl here: https://issues.apache.org/jira/browse/AMQ-8320 |
No. App can use JMS 1.1 or JMS 2.0 API (EDIT: actually yes, the issue will only arise if they use 2.0 to be fair...which would be more likely given its the aim of this change). Configuring DeliveryDelay at all is not required. App sends JMS 1.1 providers message object, through JMS 2.0 client (which tolerates 1.1 messages). After this change as-is, making the '1.1 provider' use the 2.0 API and throw UOE for impl, that will fail all the time during send. Its possible for a JMS 2.0 client to work upon and send 1.1 message objects, tolerating that the message.setDeliveryTime() method is defined but is not being implemented by the older object (as clearly it didnt exist before, and so couldnt be implemented, but backwards compatibility is the name of the game for JMS). The JMS reference impl definitely did this. Its possible for a JMS 2.0 client to tolerate being used only with the 1.1 API jar and for the message.setDeliveryTime() method to not actually be defined at all at runtime. I dont recall if the RI did this. Qpid JMS for example does both of these things. It will let you send a 1.1 message through it, and it will operate using the 1.1 API jar. It simply made sense to support when transitioning specs, not dropping support for using it with existing 1.1 providers (e.g 5.x) or existing applications, when they couldn't actually be doing anything that wasnt still supported as all that API all remained precisely for compatibility. Those are very different situations from the method being defined, implemented and explicitly throwing UOE, and I dont expect anything to really handle that it really makes no sense for it to ever do so, especially for a long setter that is simply for recording 'what actually happened during send'. Note that all sends will fail, regardless whether a delivery delay is configured on the producer, as a value is always being set on the sent messages to either indicate the actual delivery time or ensure it records there was no delay (as the sent message object may previously have had a deliveryTime value, so that needs cleared to ensure the getter reflects reality).
The 4th happens because its an entirely different situation than what #3 is handling. A method not existing / not being implemented and that being handled, is quite different than it actually being there but deliberately throwing.
There is no delay unless the feature is implemented (which would remove any need for a discussion), so there cant really be a sillent wiping. The message setters dont control the delay as already discussed, and why would anyone expect the message to be reporting a non-zero delivery time after send, when producer.setDeliveryDelay(..) is throwing to indicate a delay cant actually be configured? |
|
As an example, the Qpid JMS has a test module that uses ActiveMQ 5 for some tests and those include a 'foreign message' test. Currently it works with 5.16.2. Modifying the module now to use the snapshot from this PR, it then fails that test due to the change I've noted here. |
Cool, what is the link to the test code? Test output? |
The test code is at https://github.com/apache/qpid-jms/blob/1.1.0/qpid-jms-interop-tests/qpid-jms-activemq-tests/src/test/java/org/apache/qpid/jms/producer/JmsMessageProducerTest.java#L177. Its very simple, 'create foreign 5.x message object, try to send + receive message'. It fails to send on line 200, due to JMSDeliveryTime UOE from the message. For completeness, this is the change I made to try out the changes: gemmellr/qpid-jms@516462f |
Without a stacktrace, I'm estimating that this is the method throwing the exception: Yeah, so in that code it could be argued that the 'hasDelay' marker variable (used 7 lines earlier in L907) should be used before calling the setJMSDeliveryTime in the private setForeignMessageDeliveryTime method. That method presumes that it should call the setJMSDeliveryTime even if there hasn't been a DeliveryDelay specified. Either approach (throw UOE or swallow expected Delivery Delays) will present downside to varying users and interests. I contend that the current UOE approach matches several project goals:
There is a dedicated JIRA for DeliveryDelay implentation discussion here: AMQ-8320 |
It is set for every message sent because it should be. The [foreign] message object may already have a populated JMSDeliveryTime value so even if there is no delay being applied during this send call, the JMSDeliveryTime value should still be stamped appropriately on the sent message object to ensure it is correctly reflecting this send. (The 'hasDelay' variable is used for something quite different, but obviously related).
The message.set/getJMSDeliveryTime would just work as they are meant to, allowing their use to simply report the values, and not breaking usage of 5.x message object with other providers, its hardly 'swallowing expected delays'. The message setter and getter can be trivially handled in 5.x (getting and setting a basic long field), without the producer.setDeliveryDelay() method or delivery delay feature being implemented in 5.x, i.e. the producer.setDeliveryDelay() would still throw until actually implemented.
I would say that the mailing list discussion has not addressed this particular aspect, and it highly likely noone thought of it until I pointed it out. As a breaking change I think it runs rather counter to the entire idea of using the newer api jar being 'more compatible' as some seem to think.
Fixing the message method issue would use less characters than throwing the exceptions would so arguably it would be more concise. Weve spent many many multiples of the time it would take to fix this, just discussing. I'm done, break what you like. |
|
I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec. That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be). How does that sound? The action items there would be:
(admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that does not throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner) |
|
It’s not about qpid its that you’re breaking cross protocol stuff. Qpid is just showing those breaks up. The fact the activemq test didn’t pick up, is just a miss but thankfully the qpid projects tests did.
I think it ties back to trying to fudge in jms 2.0 client side without proper changes broker side, e.g. maybe this could be sorted by adding the correct jms 2.0 mapping to AMQP broker side to sort correct protocol support.
…Sent from my iPad
On 4 Aug 2021, at 21:29, ehossack-aws ***@***.***> wrote:
I'm wondering if a constructive way to move this conversation forward, and JMS 2.0 support in general, would be to define a series of JMS20CompatibilityTests, that utilize the APIs and act accordingly.
The tests could assert that UOEs are thrown when expected, and certain functionality works across 2.0 APIs when UOEs are not expected.
A feature-complete reiteration of the JMS spec might not have value, but there's certainly core areas that could be covered.
As new support comes for methods (subtasks implemented of https://issues.apache.org/jira/browse/AMQ-7309) those tests could be altered as functional, and potentially all tests moved or relocated upon feature-complete spec.
That way too, we're talking about specific test cases in ActiveMQ and not across QPID (valid to support though they may be).
How does that sound? The action items there would be:
@mattrpav to add a series of basic tests that cover the altered APIs
@gemmellr to suggest a test (either before or after matt adds the tests) that could cover the use case in a clear/concise manner (e.g. Build/Operate/Check), that would move the convo forward
(admittedly as a passive watcher of this PR, I'm having a hard time following your suggestions, i.e. understanding how the implementation of delivery delay that does not throw exceptions in the case of trying to mutate or access data that isn't supported could provide return values with meaning, and the linked test case I'm assuming is failing due to this implementation decision: https://github.com/apache/qpid-jms/blob/6775a2e027d8963cda4f1a7dc75b115c5def4d47/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java#L999 - in which case QPID could choose to catch the UOE in a similar manner)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
Maybe stupid but in tomee to have jms2 api support i wrapped amq5 in "TomEEAMQ" classes. This is pretty straight forward and fast to do without any regression. Only change in current code can be in factories with a version config to switch between impl but at the end it is mainly delegation code and works well. |
|
@rmannibucau yes, it's what I have in mind as well, as a first step (and what I started). @mattrpav took more "ambitious" approach. |
@tabish121 activemq-camel removal PR is here: #701 |
|
@mattrpav can you please squash all commits in one ? If you don't know how to do it, just ping me on slack or I can do it for you. Thanks. |
|
I'm quicly fixing https://issues.apache.org/jira/browse/AMQ-8408 to perform a new test series on this PR. I will merge when all tests are OK (including OSGi/Karaf ones). |
activemq-jms-pool/src/main/java/org/apache/activemq/jms/pool/PooledConnection.java
Outdated
Show resolved
Hide resolved
activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
Outdated
Show resolved
Hide resolved
activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
Outdated
Show resolved
Hide resolved
activemq-ra/src/main/java/org/apache/activemq/ra/ActiveMQConnectionFactory.java
Outdated
Show resolved
Hide resolved
| @@ -32,7 +32,6 @@ | |||
| <bundle dependency='true'>mvn:org.apache.servicemix.bundles/org.apache.servicemix.bundles.jaxb-impl/${jaxb-bundle-version}</bundle> | |||
| <bundle>mvn:org.apache.commons/commons-pool2/${commons-pool2-version}</bundle> | |||
| <bundle>mvn:commons-net/commons-net/${commons-net-version}</bundle> | |||
| <bundle dependency="true">mvn:org.apache.zookeeper/zookeeper/${zookeeper-version}</bundle> | |||
There was a problem hiding this comment.
? why is this being removed as part of jms change?
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
Outdated
Show resolved
Hide resolved
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
Outdated
Show resolved
Hide resolved
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
Outdated
Show resolved
Hide resolved
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
Show resolved
Hide resolved
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducerSupport.java
Outdated
Show resolved
Hide resolved
activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
Outdated
Show resolved
Hide resolved
activemq-client/src/main/java/org/apache/activemq/ActiveMQConnectionFactory.java
Show resolved
Hide resolved
|
seems some code style clean up needs to be done in general before merge, indentation a little over the place in many files. Like wise, why as part of JMS change is zookeeper being removed? |
Zookeeper was already removed from the code base via a different PR. The only change included here is housekeeping the zookeeper entry from the Karaf features file. Found it while adding the geronimo-jms 2.0 dep. |
I'm on it |
|
@mattrpav thanks for sorting the formatting. |
- API update only - Throw UnsupportedOperationException - Disable activemq-camel from build - Formatting fixes - Use geronimo-jms for osgi-related artifacts - Fix features.xml invalid xml header - Add a unit test to confirm JMS 2.0 methods for phase 1 (throw UnsupportedOperationException) - Add deliveryTime field to Message - Minor formatting fixes
Revert "[AMQ-7309] Update to jakarta.jms/jakarta.jms-api:2.0.3 (#682)"
Planned changes: