[AMQ-9593] Add option to clean scheduled messages on startup

[kenliao94]

Working in progress: add an option to delete all scheduled message on startup.

[kenliao94]

Hey @mattrpav and @jbonofre I am curious on what you think about the approach here and want to clarify a few things:

1. I was debating between adding this logic to the job scheduler or the persistence layer (following the approach of deleteAllMessages). However, I noticed that SchedulerBroker doesn't expose their internal scheduler, and it has public methods that deal directly with Message (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?

2. I wasn't able to find any other non-message jobs (even job in ByteSequence type is assumed to be unmarshal into Message) scheduled on "JMS" scheduler so I assume that remove all jobs are safe. However, do you know any pitfull for simply remove all jobs?

[kenliao94]

The PR is now ready for review.

[cshannon]

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.

[kenliao94]

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

[cshannon]

Yeah that's fine, the most important thing is it's consistent for the config (which it is)

[cshannon]

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

[kenliao94]

Thanks for the feedback. I will add a unit test.

[mattrpav]

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?

[kenliao94]

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.

[jbonofre]

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 ?

[kenliao94]

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.

[cshannon]

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)

[jbonofre]

This PR looks good to me for 6.2.0, with the use of a dedicated tag. @mattrpav @cshannon @kenliao94 thoughts ?