Skip to content

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

Open
mattrpav wants to merge 1 commit intoapache:mainfrom
mattrpav:AMQ-8320
Open

WIP: [AMQ-8320] Add JMS 2.0 JMSProducer deliveryDelay support#1157
mattrpav wants to merge 1 commit intoapache:mainfrom
mattrpav:AMQ-8320

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

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants