AMQ-8464: Implement receiveBody methods in the Consumer#1543
AMQ-8464: Implement receiveBody methods in the Consumer#1543jbonofre wants to merge 1 commit intoapache:mainfrom
Conversation
activemq-client/src/main/java/org/apache/activemq/ActiveMQConsumer.java
Outdated
Show resolved
Hide resolved
|
As @tabish121 pointed out, the current implementation violates the spec I think. I think the best way to implement this is by creating a ActiveMQMessageConsumer#receiveBody that ActiveMQConsumer can delegate to. Currently, ActiveMQConsumer first delegates to ActiveMQMessageConsumer that will ack the message before reading the body. But we need to read first the body for receiveBody. |
|
@jeanouii thanks for the review. I have to resume my work on this PR. I agree that the approach you are proposing makes more sense (else the ack will be send too early). I will update accordingly. @tabish121 @jeanouii thanks for your inputs ! I will resume my work on this one. |
|
I updated this PR with a new implementation approach. |
cshannon
left a comment
There was a problem hiding this comment.
I only glanced at this and haven't reviewed the rest of the changes, but one thing that immedaitely stood out that needs to be changed is this completely breaks exception handling. JMSConsumer needs to be converted to a runtime exception but ActiveMQConsumer still implements MessageConsumer. This is a problem because anyone who is using that API may be using a try/catch and looking for JMSException that will now no longer be thrown so it's a major breaking change if client applications are catching checked exceptions that will never be thrown so we can't do that.
I think you need to do something like the approach Artemis did, where they wrap the MessageConsumer instead. See https://github.com/apache/artemis/blob/2e390b4fa0842ed8d3557a158c9ca4d204687cf3/artemis-jms-client/src/main/java/org/apache/activemq/artemis/jms/client/ActiveMQJMSConsumer.java
|
@cshannon yup, it's what I realized while working on the PR. I will update. |
| return null; | ||
| } | ||
|
|
||
| beforeMessageIsConsumed(md); |
There was a problem hiding this comment.
I think you could avoid a lot of complexity here by deferring beforeMessageIsConsumed(md); until after you've actually checked that you want to consume the message and if you can't / won't because the body is not assignable to the target you just enqueue the message back into the head of the unconsumed list as you haven't consumed it. That way you don't need to perform the various rollback work since you won't have done anything that triggers actual tracking work in the consumer.
No description provided.