[AMQ-8322] JMS 2.0 Implementation - First phase

[mattrpav]

• [x] Create JMSContext
• [x] Create JMSProducer
• [x] Create JMSConsumer
• [x] Add JMSException -> JMSRuntimeException wrapping
• [x] Add methods for working with queues
• [x] Update unit test to send-recv using JMS 2.0 API objects (JMSProducer, JMSConsumer)
• [x] JMSContext autostart handling
• [x] QueueBrowser unit tests (autostart, false, and delayed)
• [x] JMSContext multi-context and instance counting support
• [x] JMSProducer review fixes and improvements
• [x] All message types and destination types unit tests
• [x] All message property types and min/max values unit tests
• [x] JMSProducer priority and deliveryMode bounds checking and unit tests
• [x] JMSConsumer Topic durable subscription support and unit tests
• [x] Add unit tests for producer overrides and disableTimestamp handling
• [x] Update for thread safety and state locking
• [x] Add unit test and consider for disableMessageID being ignored
• [x] Add ack mode unit tests
• [x] Add message listener unit tests
• [x] Add mixed producer tests (disableTimestamp vs not)
• [x] Add producer invalid property name and value(s) unit tests
• [x] Document JMS2 page on website that disableMessageID not supported by default provider. Everything is wired through to the ActiveMQSession in the event that someone wants to swap out some protocol handler to do it.
• [x] Refactor message consumer pattern
• [x] Refactor test message listener to use countdown latch and concurrency safe collections for verification
• [x] Refactor support test class send patterns to use a builder class (MessageData) for legibility
• [x] Reduce total unit test time

[cshannon]

@gemmellr or @tabish121 - Can one (or both) of you take a look when you get a chance? Since you have had a ton of expeirnece with the JMS 2.0 spec for Qpid JMS I figured it would be good to get your eyes on this to look for anything that would break spec wise or not be compliant.

[cshannon]

For context this PR builds on this commit it looks like: https://github.com/apache/activemq/commit/67256c6

[gemmellr]

I also skimmed it and noted some things, replied to some of the comments on Tim's review. Even for a first phase it seems pretty raw/incomplete at this point, and I'd echo the near total lack of tests giving little confidence. Especially given discussion of including it in releases in a week or two, when that release has been getting promised 'soon' to folks on the users list since 2020.

[cshannon]

@mattrpav - it would probably make sense to take a look at Qpid JMS and Artemis implementations for the JMS 2.0 api. You can probably just copy a lot of what they did for JmsProducer, JmsConsumer, etc.

For example: https://github.com/apache/qpid-jms/blob/main/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsProducer.java

[gemmellr]

Note the PR build is still failing, with:

[INFO] Running org.apache.activemq.transport.http.HttpJMSMessageTest

[ERROR] Tests run: 32, Failures: 0, Errors: 4, Skipped: 0, Time elapsed: 16.633 s <<< FAILURE! - in org.apache.activemq.transport.http.HttpJMSMessageTest

[ERROR] testForeignMessage {connectURL=vm://localhost?marshal=false, deliveryMode=1, destinationType=1}(org.apache.activemq.transport.http.HttpJMSMessageTest) Time elapsed: 0.441 s <<< ERROR!

java.lang.AbstractMethodError: Receiver class org.apache.activemq.JMSMessageTest$ForeignMessage does not define or inherit an implementation of the resolved method abstract setJMSDeliveryTime(J)V of interface javax.jms.Message.

etc etc

(Dont know about the non-PR build)

[mattrpav]

@gemmellr yep, saw that. Def related to your points above. I’m cleaning that up first thing this morning.

UPDATE: Change is in for deliveryTime http unit tests passed locally (including the foreign message test). Pending Jenkins CI job to go 'green'

[mattrpav]

Grrr.. fighting with one test that passes-locally-fails-on-apache-ci.

The test totally looks like it could be something that was impacted by the changes in the PR, but the exception stack doesn't align

Test: activemq-amqp/JMSInteroperabilityTest.testQpidToOpenWireObjectMessage

javax.jms.JMSException: Failed to build body from content. Serializable class not available to broker. Reason: java.lang.ClassNotFoundException: Forbidden class java.util.UUID! This class is not trusted to be serialized as ObjectMessage payload. Please take a look at http://activemq.apache.org/objectmessage.html for more information on how to configure trusted classes.
	at org.apache.activemq.transport.amqp.JMSInteroperabilityTest.testQpidToOpenWireObjectMessage(JMSInteroperabilityTest.java:402)
Caused by: java.lang.ClassNotFoundException: Forbidden class java.util.UUID! This class is not trusted to be serialized as ObjectMessage payload. Please take a look at http://activemq.apache.org/objectmessage.html for more information on how to configure trusted classes.
	at org.apache.activemq.transport.amqp.JMSInteroperabilityTest.testQpidToOpenWireObjectMessage(JMSInteroperabilityTest.java:402)

[jbonofre]

@mattrpav it seems related to security enforcement we added. But it should fail on your machine as well.

[mattrpav]

@mattrpav it seems related to security enforcement we added. But it should fail on your machine as well.

I agree with you =)

The connection factory is setup without setting trusted packages: AmqpTestSupport#createJMSConnection()

        ActiveMQConnectionFactory factory = new ActiveMQConnectionFactory(openwireURI);
        return factory.createConnection();

.. which defaults to ClassLoadingAwareObjectInputStream#static{}

serializablePackages = System.getProperty("org.apache.activemq.SERIALIZABLE_PACKAGES","java.lang,org.apache.activemq,org.fusesource.hawtbuf,com.thoughtworks.xstream.mapper").split(",");

.. but should be overidden by AmqpTestSupport#setUp

        System.setProperty("org.apache.activemq.SERIALIZABLE_PACKAGES", "java.util");

Hmm.. that could be an order of operations problem. I'm going to change the test to add 'java.util' to the list when it instantiates the connection factory.

[gemmellr]

The value set by the property in ClassLoadingAwareObjectInputStream is a static, so updating it doesnt do anything if already set when the codepath has been first used. So the first test to use that codepath will lock the value down. If a test sets the value itself before it is ever used, then running that test in isolation will always work. If another test is run before it in the same suite in the same JVM and exercises that codepath first with a mismatched value, the subsequent tests setter will have no effect on what actually happens, and the test will fail.

[mattrpav]

@gemmellr yep. in my update to the test, I explicitly set the trusted packages list to a new list and get rid of using the system property (which may have been causing the irregularity). The ActiveMQConnectionFactory.getTrustedPackges() list is wired into the connection each time, so updates there should carry over regardless of the static block in CLAOIS

ActiveMQConnectionFactory#configureConnection(ActiveMQConnection)
         connection.setTrustedPackages(getTrustedPackages());

[gemmellr]

AH, I somehow missed the last line of your previous message :)

[mattrpav]

That amqp test fix worked. I'm going to break it out into a separate PR for consideration into 5.17.0

[mattrpav]

@gemmellr @tabish121 @cshannon @jbonofre I have gone through all the requests in previous review rounds and believe I have addressed all open items.

Please review the latest.

Thank you

[gemmellr]

I'll admit I gave up before the end as many of the comments seem to apply to a lot of the tests and I got bored seeing/saying the same things, but I cant say I saw an actual unit test despite the various comments about them. Just lots of system/integration tests running up full brokers and clients, often to check things that dont even hit the wire at all. Given a lot of this stuff just wraps existing impl it is easy to unit test those wrapping bits in isolation, system testing all of it in that way seems largely just a waste, but even more so when also repeating tests for every type of destination or message type or ack mode etc....and doing so without any unit tests that could achivee the same more quickly and give better errors on failure etc.

[mattrpav]

I'll admit I gave up before the end as many of the comments seem to apply to a lot of the tests and I got bored seeing/saying the same things, but I cant say I saw an actual unit test despite the various comments about them. Just lots of system/integration tests running up full brokers and clients, often to check things that dont even hit the wire at all. Given a lot of this stuff just wraps existing impl it is easy to unit test those wrapping bits in isolation, system testing all of it in that way seems largely just a waste, but even more so when also repeating tests for every type of destination or message type or ack mode etc....and doing so without any unit tests that could achivee the same more quickly and give better errors on failure etc.

@gemmellr thank you for your prompt review. Please characterize in more detail (or point to an example) of a unit test as you described.

I'm happy to re-work and improve test execution time and structure. Others had requested test coverage for message types and ack modes.

[mattrpav]

Unit test execution times:

Local MacBook Pro (total 32.535s):

[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.601 s - in org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.318 s - in org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 15.343 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Tests run: 220, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.216 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ProducerTest
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.057 s - in org.apache.activemq.jms2.ActiveMQJMS2ProducerTest

Apache Jenkins CI (total 43.921s):

[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.42 s - in org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.07 s - in org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 17.283 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Tests run: 220, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.782 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ProducerTest
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.366 s - in org.apache.activemq.jms2.ActiveMQJMS2ProducerTest

[gemmellr]

@gemmellr thank you for your prompt review. Please characterize in more detail (or point to an example) of a unit test as you described.

A unit test tests a particular unit, not the entire thing. The API changes that affect the Message instances for example, almost none of them actually need a message to be sent or received, they are largely orthoganl. Even the ones that you might want to system test 'for completeness', the underlying behaviour can still be unit tested more closely without a broker. The new producer API as another example, a significant chunk of that can be tested without ever creating a connection.

There are many different approaches, but a basic thing as I just covered would be I wouldnt expect the whole client or a broker to be needed at all for many of the checks for the stuff being added, which is largely just a new API for using existing functionality. The new bits can be tested in isolation. A lot of the stuff being tested never needs to hit the wire and so never needs a broker or the time wasted on starting it and connecting to it etc, or waiting on messages to be consumed to check things easily verified before they were ever actually sent etc.

I'm happy to re-work and improve test execution time and structure. Others had requested test coverage for message types and ack modes.

There are other ways to cover things. E.g unit tests. Running the same system test over and over and varying things that ultimately have almost no relation to the thing being tested, is a complete waste of time.

[gemmellr]

Local MacBook Pro (total 32.535s): Apache Jenkins CI (total 43.921s):

I'm taking it from the comments here and https://github.com/apache/activemq/pull/729#discussion_r866242620) that 32-44 seconds isnt seen as a that long of a time for this, so thats going to be the key mismatch in our views. I think thats glacial for testing the stuff actually being added here, which is almost entirely fluffy wrapping to expose existing functionality with new API. I wouldnt really want to use 30sec even supposing if it included all the actual-new-functionality / behavioural stuff that hasnt yet been implemented.

[mattrpav]

Total test time down < 13s on local MacBook Pro

[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.333 s - in org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.297 s - in org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.107 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Tests run: 220, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.882 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ProducerTest
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.057 s - in org.apache.activemq.jms2.ActiveMQJMS2ProducerTest

[mattrpav]

Full unit test suite passing

ref: https://ci-builds.apache.org/blue/organizations/jenkins/ActiveMQ%2FActiveMQ/detail/PR-729/33/pipeline

JMS 2.0 tests < 17s on Apache Jenkins

[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.88 s - in org.apache.activemq.jms2.ActiveMQJMS2AckModesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.694 s - in org.apache.activemq.jms2.ActiveMQJMS2ContextTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.196 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageListenerTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Tests run: 220, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 6.637 s - in org.apache.activemq.jms2.ActiveMQJMS2MessageTypesTest
[INFO] Running org.apache.activemq.jms2.ActiveMQJMS2ProducerTest
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.088 s - in org.apache.activemq.jms2.ActiveMQJMS2ProducerTest

[mattrpav]

Rebased against main post-5.17.2 release.

[mattrpav]

Feels like this PR is in good shape to merge. Please provide a final review for any blocking issues and provide path forward for the in-scope features. Thank you!

@cshannon @gemmellr @tabish121 @jbonofre

[gemmellr]

Not implementing the Message getBody and isBodyAssignableTo bits, and also the JMSConsumer 'recieve body' behaviour when the JMSProducer side 'send <body>' equivalent is implemented, does seem quite odd. But then to me personally so does the idea of releasing anything not considered e.g. an alpha, without so many basic new APIs and most of the core JMS 2 behavioural additions yet being implemented.

Merging may still make sense, it has been open for 9 months already and it is after all considered a first-pass...but releasing it isnt really making much more to me now than it did when 5.17.0 was being prepared 6 months ago; it hasnt really substantially changed since then. I'll be leaving further review to others, e.g JB was keen to implement it originally and tends to work on the 5.x releases.

[cshannon]

@gemmellr - Yeah, this could be merged as is and then add a follow on PR for getBody, isBodyssignable and JMSConsumer receive body. I think those should be implemented for 5.18 as no reason not to really but as you said this PR has been open for 9 months so it's time to merge it.

And your comment about the "alpha" thing is what I was getting at with my comment about communication to users. Not sure the best way to describe it but we need to make it clear this is not fully implemented and kind of a preview or alpha/beta of JMS 2.0 changes or however we want to call it.

[mattrpav]

Sounds good. Merging this now and then will get started on the next round of features.