-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[AMQ-9593] Add option to clean scheduled messages on startup #1352
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
Conversation
Hey @mattrpav and @jbonofre I am curious on what you think about the approach here and want to clarify a few things:
-
I was debating between adding this logic to the job scheduler or the persistence layer (following the approach of deleteAllMessages). However, I noticed that
SchedulerBrokerdoesn't expose their internal scheduler, and it has public methods that deal directly withMessage(initially I thought SchedulerBroker is not "message aware" but only "job aware". So I decided to add the logic in SB. What do you think? -
I wasn't able to find any other non-message jobs (even job in
ByteSequencetype is assumed to be unmarshal intoMessage) scheduled on "JMS" scheduler so I assume that remove all jobs are safe. However, do you know any pitfull for simply remove all jobs?
15b6106 to
c7c4d1a
Compare
The PR is now ready for review.
It might be be better to move the cleanup to the persistence layer just because that's how KahaDB handles clearing messages (as was pointed out). But I don't really care too much either way because the flag on the BrokerService is consistent with the other flag so for users it's still a consistent way to configure it. I'll let @mattrpav and @jbonofre comment to see if they have an opinion.
@cshannon yeah I was doing that as my first approach. But then I realize I need to somehow expose (by changing the visibility of the field) the name of the scheduler ("JMS") used by SchedulerBroker. I didn't like that approach because it creates some sort of coupling. So I decided to handle it where no visibility needed to change.
Yeah that's fine, the most important thing is it's consistent for the config (which it is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look and it generally seems ok but it would be good to have a unit test to verify it works. Something where scheduled messages are added, restart, and verify they are removed etc
Thanks for the feedback. I will add a unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scheduled message support is part of the JMS 3.1/2.0 spec. I'm not loving the separate config flag. We don't have a 'deleteAllDurableTopicMessagesOnStartup' flag. It is 'all' messages on startup.
I'm thinking it would be better to have a flag to have the scheduler ignore the deleteAllMessagesOnStartup.
Possible path forward:
v6.x - Scheduler ignores deleteAllMessagesOnStartup by default (to maintain existing behavior)
v7.x - Scheduler honors deleteAllMessagesOnStartup by default
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you are coming from. Initially I would expect deleteAllMessagesOnStartup will include scheduled messages. I even opened a Jira ticket on it. https://issues.apache.org/jira/projects/AMQ/issues/AMQ-9572?filter=allissues from @jbonofre feedback we should have a separated flag for scheduled messages.
Regarding the proposal in v7.x I think if we have the consensus that deleteAllMessagesOnStartup should include scheduled messages then it makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still in favor on a dedicated flag here, because the semantic is different so the behavior is.
To reduce the "side effect" for existing users on ActiveMQ 6.x, I'm in favor of deleteAllScheduledMessagesOnStartup. We can revisit this for 7.x.
@mattrpav @kenliao94 thoughts ?
ec39578 to
6747504
Compare
6747504 to
0652458
Compare
Hey @mattrpav @cshannon and @jbonofre what do you think about this PR? Should we merge it for 6.1.5 release?
I recently added a unit test as well.
It should be 6.2.0 just like #1288 , see #1288 (comment) and #1288 (comment)
This PR looks good to me for 6.2.0, with the use of a dedicated tag. @mattrpav @cshannon @kenliao94 thoughts ?
hey @jbonofre sounds good to me! When you said "dedicated tag" do you mean a attribute on the XML as it is today?
Working in progress: add an option to delete all scheduled message on startup.