Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: [AMQ-8320] Add JMS 2.0 JMSProducer deliveryDelay support #1157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattrpav
Copy link
Contributor

@mattrpav mattrpav commented Feb 23, 2024

  • Improve Message object reuse
  • Add unit test to confirm

ref: #982

@mattrpav mattrpav self-assigned this Feb 23, 2024
@gemmellr
Copy link
Member

Seems like the same code as the old PR, so same comment from before:

Since message objects can be reused/resent, and might already have an existing value for JMSDeliveryTime, it should always be setting the JMSDeliveryTime value on send even if there isnt a delay, to ensure the JMSDeliveryTime value on the message after send reflects behaviour of that send and not some previous send or receive.

That doesnt mean it has to transit a delivery time on the wire...ideally it wouldnt if there is no delay being requested for this send...just that after send the local message object should report the delivery time for that send. Elsewhere I added an internal message method to set the value and note whether it is for transmission.

It doesnt feel like it should be setting a message property at all, only a header. I don't expect the TCK tests which check additional properties arent being added to the messages are using delivery-delay at the time..but they could. I know the older 'message property extension' scheduling stuff uses one, but its not clear to me this needs to. They could just be treated as separate mechanisms, which they kind of are already as you have to choose to respect the new value on the producer or the old one on the message. Theres no reason the broker couldnt do the same, and this new mechanism essentially ignore that old prop.

@gemmellr
Copy link
Member

It also now occurs to me it may not handle foreign providers Message properly since it seemingly doesnt call setJMSDeliveryTime at all on send (unless thats being done elsewhere already...though doesnt seem obvious it could know the value to use).

Although the PR title makes it clear that it does, it also seems weird this change only covers the JMSProducer API related code, and misses out the MessageProducer related API, when delivery delay support applies equally to both (and the underlying send code is used for both).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants