[AMQ-8322] JMS 2.0 Implementation - First phase #729

mattrpav · 2021-11-29T20:28:17Z

cshannon · 2022-01-25T19:09:57Z

@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 · 2022-01-25T19:15:37Z

For context this PR builds on this commit it looks like: 67256c6

tabish121

I did a quick pass and left some comments on some spec violation or other areas that could be improved. Testing is minimal and doesn't provide much confidence in the correctness of the implementation so I'd suggest that as a big area of improvement.

activemq-client/src/main/java/org/apache/activemq/ActiveMQContext.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQProducer.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQContext.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQConsumer.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQProducer.java

gemmellr · 2022-01-26T12:25:50Z

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 · 2022-01-26T14:14:19Z

@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

I also find it surprising there isnt a load of (/any?) unit tests at this point, especially as many of the additions are largely just wrapping an existing objects from the classic API and so should be very easily unit testable in isolation.

gemmellr · 2022-02-21T11:41:15Z

activemq-client/src/main/java/org/apache/activemq/ActiveMQSession.java

+                if(!disableMessageID && !producer.getDisableMessageID()) {
+                    message.setJMSMessageID(msg.getMessageId().toString());
+                }


If it is disabled it should be clearing the value that might already exist, similar to the timestamp handling does above.

activemq-client/src/main/java/org/apache/activemq/ActiveMQSession.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQContext.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQProducer.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQSession.java

gemmellr · 2022-02-22T11:41:46Z

activemq-client/src/main/java/org/apache/activemq/ActiveMQSession.java

+            }
+
+            // TODO: AMQ-8500 - update this when openwire supports JMSDeliveryTime
+            message.setJMSDeliveryTime(timeStamp);


This still seems to be the 'original message' at this point, so note this will throw if it was given a foreign providers JMS 1.1 impl message object, which naturally doesnt implement the method since it doesnt exist for them (remembering back to the previous discussion around why the message set/get methods shouldnt throw UOE to avoid breaking other providers, and how a 2.0 provider can allow working with anothers existing 1.1 messages).

I'm not following you on what the recommended approach here is. You mentioned before that the field is required and should be hydrated with the timestamp value.

ActiveMQ producer message (and foreign v2) needs value for client after message has been processed through the producer

ActiveMQ consumer message (and foreign v2) needs value for client after message has been received

To handle a v1 message we'd have to add reflection here and check existence of the field before assignment. That sounds very expensive for every message operation. It would not be safe to cache any of the reflection objects.

What am I missing?

Not much. You already have to check that it is a foreign message, and are already going to the expense of converting it, this cost need only apply in those cases. Relative to e.g the round trip to the broker for any reliable messaging operation the cost is likely not that significant. If you think barffing and throwing an exception is better, I would disagree.

The consumer bit has to happen either way. It can synthesize the value from the timestamp (if present) if it doesnt have a specific differnet value.

For the record, I try to avoid barfing exceptions whenever possible ;-)

It was unexpected to me that this level of reflection would be the solution. I'm working on it now. That foreign message test over in http is helpful and will provide good feedback

@gemmellr what are your thoughts on returning JMSTimestamp vs zero for the v1 use case?

getFromMessageDeliveryTime(Message fromMessage)

Check for the existence of the getJMSDeliveryTime method

If so (ie.. v2 message) return value

If not, return the JMSTimestamp value

Earlier we had discussed using JMSTimestamp as the fallback since it is a required field. This would align the behavior across ActiveMQ usage. It looks like qpid is returning 0 in this case. Is there a specific reason to return 0 here?

Thanks

I should probably also note that this call will fail even for a native 5.x message, if the 1.1 API is the primary interface on the classpath, as the method 'wont exist' even though 5.x implements it. To avoid that you need to use an ActiveMQMessage impl ref to call the method rather than the Message interface.

Got it. I'm working on both the set and the get side of it. Can I get your thoughts on the above? JMSTimestamp vs zero for non-JMS 2.0 messages. Thanks

I could go other way on that one. Both are about as non-meaningful as each other, as they will be in the past from the recievers perspective. 0 is like the 'defaullt', ala expiration etc. Tbh I'm not sure I actually know of a reason to look at the value after receiving a message, except as part of an implementation test hehe.

I think from your 'getFromMessageDeliveryTime(Message fromMessage)' inclusion you mean during the send though. In that case, strictly speaking I guess you dont necessarily need to get the value as you are about to set the value, so the original is actually redundant.

Agreed that it is tough to find a meaningful use case on the receiver side. Maybe detecting clock skew? Yeah.. its narrow at best. I'll go with JMSTimestamp alignment for now and see how that feels once the tests are flushed out. I think it'll have the small benefit of making the documentation more straight-foward.

gemmellr · 2022-02-22T13:30:03Z

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 · 2022-02-22T13:48:06Z

@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 · 2022-02-23T14:51:29Z

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 · 2022-02-23T14:56:30Z

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

mattrpav · 2022-02-23T15:01:44Z

@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 · 2022-02-23T16:27:29Z

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 · 2022-02-23T17:29:49Z

@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 · 2022-02-23T17:45:17Z

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

mattrpav · 2022-02-23T18:53:50Z

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

tabish121

Added some comments for various things spotted during another pass through the code

activemq-client/src/main/java/org/apache/activemq/ActiveMQConsumer.java

activemq-client/src/main/java/org/apache/activemq/ActiveMQProducer.java

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2ContextTest.java

mattrpav · 2022-05-05T13:46:30Z

@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

activemq-client/src/main/java/org/apache/activemq/ActiveMQProducer.java

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2AckModesTest.java

gemmellr · 2022-05-05T14:53:15Z

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2TestSupport.java

+import javax.jms.StreamMessage;
+import javax.jms.TextMessage;
+
+public class ActiveMQJMS2TestSupport {


This class, and those using it, are screaming for some use of constants for argument values and property names etc.

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2AckModesTest.java

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2MessageListenerTest.java

gemmellr · 2022-05-05T16:19:34Z

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2MessageTypesTest.java

+            do {
+                recvMessage = jmsConsumer.receive(500l);
+                if(recvMessage != null) {
+                    recvMessages.add(recvMessage);
+                }
+
+                if(recvMessages.size() == 10) {
+                    done = true;
+                }
+                loopCount++;
+            } while (loopCount <= maxLoops && !done);


Here and in all the other tests doing similar, a simple loop that allowed 1 shot at recieving a message (with a longer more suitable timeout) would seem simpler and fail better if needed ( e.g. "failed to get message: 2") than these loops with massive extra attempts headroom.

I agree that this test pattern needs to be improved. I had brought this up before when working tests for 5.17.x so I think now would be a good time to rewrite this as it's re-used in several tests.

I will add at the very least you can at least turn it into a helper method and re-use the code that way it can be easily refined or tuned later vs where it's copy and pasted all over the place now (that was my main complaint from 5.17.x where the same do/while was copy pasted over and over so if you wanted to change the design it would be awful to fix)

gemmellr · 2022-05-05T16:27:54Z

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2MessageTypesTest.java

+            int foundCount = 0;
+            for(int validatedPriority=0;validatedPriority<10;validatedPriority++) {
+                for(javax.jms.Message tmpMessage : recvMessages) {
+                    if(tmpMessage.getJMSPriority() == validatedPriority) {
+                        ActiveMQJMS2TestSupport.validateMessagePriority(tmpMessage, messageType, messagePayload, validatedPriority);
+                        foundCount++;
+                    }
+                }
+            }
+            assertEquals(Integer.valueOf(10), Integer.valueOf(foundCount));


Adding the message priorities to a TreeSet or sopmething and asserting it contains 0 to 9 might be simpler and clearer. Or you could create the consumer after they were sent, and jsut expect them in reverse (i.e priority) order and validate on arrival.

gemmellr · 2022-05-05T16:34:11Z

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2MessageTypesTest.java

+                {"queue", "bytes"},
+                {"queue", "map"},
+                {"queue", "object"},
+                {"queue", "stream"},
+                {"queue", "text"},
+                {"topic", "bytes"},
+                {"topic", "map"},
+                {"topic", "object"},
+                {"topic", "stream"},
+                {"topic", "text"},
+                {"temp-queue", "bytes"},
+                {"temp-queue", "map"},
+                {"temp-queue", "object"},
+                {"temp-queue", "stream"},
+                {"temp-queue", "text"},
+                {"temp-topic", "bytes"},
+                {"temp-topic", "map"},
+                {"temp-topic", "object"},
+                {"temp-topic", "stream"},
+                {"temp-topic", "text"},


This type of all-combinations stuff seems quite unecessary in many/most of the cases, but especially this class given much of its tests really have little to do with where it is sent or what type of message it is, often being implemented in the base message class and in many cases not even needing wire activity to check behaviour. A good example of where actual unit tests for coverage would be good rather than many unecessary system tests.

I don't really see it as that big of a deal to test all the permutations as long as it isn't too slow. Based on the output it seems pretty quick so I'd rather check everything and than miss something. The idea of mocking and having unit tests vs system tests is a good point though.

Look at what is being tested, it has basically nothing to do with different message types, certainly not different destination types. Its mostly a waste of time (3-7sec on examples given). Even if it did have something to do with those, coverage would be better achieved far faster with specific unit tests. Add a system test or two (not 20) if needed to check the already-tested units aren't somehow wired together backwards or not at all.

"as long as isnt too slow" somewhat depends on your view. Sure, 3-7 seconds isnt much in itself, but it is when considering all thats actually being verified here. 3-7sec seems orders of magnitude too slow to me for whats being tested. A few seconds here, a few seconds there, is exactly how to end up with the test suite 5.x currently has. The problem cant ever get any better if keeping doing the same things and continually making it worse.

gemmellr · 2022-05-05T16:35:11Z

activemq-unit-tests/src/test/java/org/apache/activemq/jms2/ActiveMQJMS2MessageTypesTest.java

+
+            int validatedCount = 0;
+            for(javax.jms.Message tmpMessage : recvMessages) {
+                ActiveMQJMS2TestSupport.validateMessageDisableMessageID(tmpMessage, messageType, messagePayload, jmsMessageID);


What is this validating exactly? That it actually has a messageID since disableMessageID isnt supported?

Yes, for completeness against the spec changes. This is negative testing the optional DisableMessageID support. Also, in the (unlikely) event that an effort comes along to add support for messageID-less delivery these will fail.

gemmellr · 2022-05-05T16:51:20Z

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 · 2022-05-05T16:59:33Z

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 · 2022-05-05T18:53:55Z

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 · 2022-05-06T11:16:49Z

@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 · 2022-05-06T11:52:23Z

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

I'm taking it from the comments here and #729 (comment)) 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 · 2022-06-29T17:38:13Z

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 · 2022-07-15T13:03:20Z

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

… first pass

mattrpav · 2022-08-30T14:30:51Z

Rebased against main post-5.17.2 release.

mattrpav · 2022-08-30T15:24:44Z

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

cshannon

I reviewed this and overall it looks pretty good to me but also want to hear what everyone else thinks too.

I think the main thing I noticed and wanted to ask before "approving" this was is there a reason not to implement getBody() or isBodyAssignableTo() methods now? Seems like we could support this.

activemq/activemq-client/src/main/java/org/apache/activemq/command/ActiveMQMessage.java

Lines 798 to 806 in 77fa2f3

    
           @Override 
        
           public <T> T getBody(Class<T> c) throws JMSException { 
        
                  throw new UnsupportedOperationException("getBody(Class<T>) is not supported"); 
        
           } 
        
           @Override 
        
           public boolean isBodyAssignableTo(Class c) throws JMSException { 
        
                  throw new UnsupportedOperationException("isBodyAssignableTo(Class) is not supported"); 
        
           }

Another thing is how we phrase the wording to users when this is released. We want to make sure to put out that this is just a first version of client changes and that not everything is fully supported and it's being done incrementally. Obviously very notable things are missing like shared subscriptions. There could also be some bugs or problems that show up that were missed when it's out there so we want to make sure people are aware of that.

gemmellr · 2022-08-31T12:16:02Z

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 · 2022-08-31T12:43:52Z

@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.

cshannon

Approving this as I think we are good to merge for now and then can address the comments I made about the getBody() impl in another PR

mattrpav · 2022-09-02T13:19:20Z

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

mattrpav self-assigned this Nov 29, 2021

mattrpav changed the title ~~[AMQ-8322] JMS2 Implementation - First pass~~ [AMQ-8322] JMS2 Implementation - First phase Nov 29, 2021

mattrpav changed the title ~~[AMQ-8322] JMS2 Implementation - First phase~~ [AMQ-8322] JMS 2.0 Implementation - First phase Nov 29, 2021

mattrpav requested review from cshannon and jbonofre December 2, 2021 23:46

cshannon requested a review from tabish121 January 25, 2022 19:06

tabish121 reviewed Jan 25, 2022

View reviewed changes

gemmellr reviewed Jan 26, 2022

View reviewed changes

activemq-client/src/main/java/org/apache/activemq/ActiveMQProducer.java Outdated Show resolved Hide resolved

gemmellr reviewed Feb 21, 2022

View reviewed changes

gemmellr reviewed Feb 22, 2022

View reviewed changes

activemq-client/src/main/java/org/apache/activemq/ActiveMQSession.java Show resolved Hide resolved

gemmellr reviewed Feb 22, 2022

View reviewed changes

asfgit force-pushed the main branch from 98c6595 to 80d4804 Compare February 23, 2022 15:37

mattrpav requested a review from tabish121 February 23, 2022 19:17

tabish121 reviewed Feb 24, 2022

View reviewed changes

mattrpav requested a review from tabish121 May 5, 2022 13:44

gemmellr reviewed May 5, 2022

View reviewed changes

[AMQ-8322] JMS2 Implementation - JMSContext, JMSProducer, JMSConsumer…

77fa2f3

… first pass

cshannon reviewed Aug 31, 2022

View reviewed changes

cshannon self-requested a review August 31, 2022 12:52

cshannon approved these changes Aug 31, 2022

View reviewed changes

mattrpav merged commit ead9533 into apache:main Sep 2, 2022

	@Override
	public <T> T getBody(Class<T> c) throws JMSException {
	throw new UnsupportedOperationException("getBody(Class<T>) is not supported");
	}

	@Override
	public boolean isBodyAssignableTo(Class c) throws JMSException {
	throw new UnsupportedOperationException("isBodyAssignableTo(Class) is not supported");
	}

[AMQ-8322] JMS 2.0 Implementation - First phase #729

[AMQ-8322] JMS 2.0 Implementation - First phase #729

Conversation

mattrpav commented Nov 29, 2021 • edited

cshannon commented Jan 25, 2022

cshannon commented Jan 25, 2022

tabish121 left a comment

Choose a reason for hiding this comment

gemmellr commented Jan 26, 2022

cshannon commented Jan 26, 2022

gemmellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemmellr Feb 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemmellr commented Feb 22, 2022

mattrpav commented Feb 22, 2022 • edited

mattrpav commented Feb 23, 2022

jbonofre commented Feb 23, 2022

mattrpav commented Feb 23, 2022 • edited

gemmellr commented Feb 23, 2022

mattrpav commented Feb 23, 2022

gemmellr commented Feb 23, 2022

mattrpav commented Feb 23, 2022 • edited

tabish121 left a comment

Choose a reason for hiding this comment

mattrpav commented May 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemmellr commented May 5, 2022

mattrpav commented May 5, 2022

mattrpav commented May 5, 2022 • edited

gemmellr commented May 6, 2022 • edited

gemmellr commented May 6, 2022

mattrpav commented Jun 29, 2022 • edited

mattrpav commented Jul 15, 2022 • edited

mattrpav commented Aug 30, 2022

mattrpav commented Aug 30, 2022

cshannon left a comment • edited

Choose a reason for hiding this comment

gemmellr commented Aug 31, 2022 • edited

cshannon commented Aug 31, 2022

cshannon left a comment

Choose a reason for hiding this comment

mattrpav commented Sep 2, 2022

mattrpav commented Nov 29, 2021 •

edited

gemmellr Feb 22, 2022 •

edited

mattrpav commented Feb 22, 2022 •

edited

mattrpav commented Feb 23, 2022 •

edited

mattrpav commented Feb 23, 2022 •

edited

mattrpav commented May 5, 2022 •

edited

gemmellr commented May 6, 2022 •

edited

mattrpav commented Jun 29, 2022 •

edited

mattrpav commented Jul 15, 2022 •

edited

cshannon left a comment •

edited

gemmellr commented Aug 31, 2022 •

edited