jbonofre commented on code in PR #1809:
URL: https://github.com/apache/activemq/pull/1809#discussion_r2964190203
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducer.java:
##########
@@ -219,6 +221,25 @@ protected void checkClosed() throws IllegalStateException {
* @see jakarta.jms.Session#createProducer
* @since 1.1
*/
+
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
+ checkClosed();
+
+ // Jakarta 3.1 Compliance Guard
+ if (deliveryDelay < 0 && session.connection.isStrictCompliance()) {
+ throw new jakarta.jms.MessageFormatException("Delivery delay
cannot be negative when strictCompliance is enabled.");
Review Comment:
This is wrong imho.
Jakarta Messaging 3.1 spec (`MessageProducer.setDeliveryDelay`) doesn't
mandate `MessageFormatException` for invalid arguments.
`MessageFormatException` is for message body/property type conversion issues
(like in `setBody`).
An `IllegalArgumentException` or `JMSException` (generic) would be more
appropriate.
The JMS Spec says nothing about negative delays being illegal, the behavior
is undefined and it's up to us to decide the validation logic.
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducer.java:
##########
@@ -219,6 +221,25 @@ protected void checkClosed() throws IllegalStateException {
* @see jakarta.jms.Session#createProducer
* @since 1.1
*/
+
+ @Override
+ public void setDeliveryDelay(long deliveryDelay) throws JMSException {
Review Comment:
Rather than defining here, why not updated `ActiveMQMessageProducerSupport` ?
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducer.java:
##########
@@ -84,6 +84,8 @@ public class ActiveMQMessageProducer extends
ActiveMQMessageProducerSupport impl
private MessageTransformer transformer;
private MemoryUsage producerWindow;
+ protected long deliveryDelay = 0;
Review Comment:
`deliveryDelay` is not used in `send()`.
It means that it's not effective.
To be effective, the delay needs to be applied during message dispatch (so
either passing it through to the session send, or setting `AMQ_SCHEDULED_DELAY`
on the message).
Also `ActiveMQProducer.setDeliveryDelay()` (for JMS 2.0 `JMSProducer`) is
not updated and still throws `UnsupportedOperationException`.
I think it's better to have the PR atomic and consistent.
##########
activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageProducer.java:
##########
@@ -84,6 +84,8 @@ public class ActiveMQMessageProducer extends
ActiveMQMessageProducerSupport impl
private MessageTransformer transformer;
private MemoryUsage producerWindow;
+ protected long deliveryDelay = 0;
Review Comment:
Do we need really `protected` here ?
As we have getter/setter access, we can go with `private` and using mock in
the test, right ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
