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

ARTEMIS-4768 _AMQ_SCHED_DELIVERY msg prop lost after broker restart #4929

Merged
merged 1 commit into from
May 22, 2024

Conversation

jbertram
Copy link
Contributor

@jbertram jbertram commented May 8, 2024

No description provided.

@@ -243,10 +243,6 @@ public void handleAddMessage(Map<Long, Map<Long, AddMessageRecord>> queueMap) th
MessageReference ref = postOffice.reload(record.getMessage(), queue, null);

ref.setDeliveryCount(record.getDeliveryCount());

if (scheduledDeliveryTime != 0) {
record.getMessage().setScheduledDeliveryTime(0L);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was on purpose. If you are reloading it it shouldn't be scheduled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look a few lines up from the code I removed you'll see this:

if (scheduledDeliveryTime != 0) {
   record.getMessage().setScheduledDeliveryTime(scheduledDeliveryTime);
}

So first we set it to scheduledDeliveryTime and then we set it to 0. One of these must be wrong. I believe the one I removed in the wrong one.

There is another block still further up that does this:

if (scheduledDeliveryTime != 0 && scheduledDeliveryTime <= currentTime) {
   scheduledDeliveryTime = 0;
   record.getMessage().setScheduledDeliveryTime(0L);
}

Reloading a message whose scheduled delivery time is already passed should not be scheduled. Is this what you were referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clebertsuconic, can you take another look at this before 2.34.0?

@jbertram jbertram merged commit 7e151ee into apache:main May 22, 2024
6 checks passed
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