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

Add custom TriggerContext to ReschedulingRunnable #23715

Closed
wants to merge 1 commit into from

Conversation

MaciejGorczyca
Copy link

@MaciejGorczyca MaciejGorczyca commented Sep 26, 2019

Changes based on:

Allow ReschedulingRunnable to receive a TriggerContext on creation #19475

I needed a way to resume work after a downtime - lets say I need to do daily reports based on what happened during that day. It doesn't matter if the report is delayed as long as it will be there when the system goes back online. With changes, you can easily handle this situation by finding last report date and creating triggerContext based on that time AND passing CronTrigger with argument fixedRate set to true.

It takes away burden of having to manually sync up on startup with custom logic (apart from finding last date), it prevents cases where you have rows with date 10.05, 11.05 and then 14.05, because new Cron fired task for day 14.05 but your catchup logic didn't finish yet the days between 11.05 and 14.05 (unless you write logic to handle "holes" between dates).

Example:

final String cron = "0/10 * * ? * *";
final CronTrigger cronTrigger = new CronTrigger(cron, true);

// quickly revert time by 1 minute for demonstration purposes
Calendar calendar = Calendar.getInstance();
calendar.set(Calendar.MINUTE, calendar.get(Calendar.MINUTE) - 1);
SimpleTriggerContext simpleTriggerContext = new SimpleTriggerContext(calendar.getTime(), calendar.getTime(), calendar.getTime());

// quickly prepare task for demonstration purposes
Thread task = new Thread(() -> {
    logger.debug("executed scheduled for {} with scheduled time {}", cron, cronTrigger.nextExecutionTime(simpleTriggerContext));
});
taskScheduler.schedule(task, cronTrigger, simpleTriggerContext));

The output for printing execution times will be:

2019-09-25 14:06:52.903 DEBUG 15336 --- [           main] project.scheduler.SchedulerService         : Scheduling test cron 0/10 * * ? * *
2019-09-25 14:06:52.905 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:06:00 UTC 2019
2019-09-25 14:06:52.906 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:06:10 UTC 2019
2019-09-25 14:06:52.906 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:06:20 UTC 2019
2019-09-25 14:06:52.906 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:06:30 UTC 2019
2019-09-25 14:06:52.906 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:06:40 UTC 2019
2019-09-25 14:06:52.906 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:06:50 UTC 2019
2019-09-25 14:07:00.001 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:07:00 UTC 2019
2019-09-25 14:07:10.002 DEBUG 15336 --- [taskScheduler-1] project.scheduler.SchedulerService         : executed scheduled for 0/10 * * ? * * with scheduled time Wed Sep 25 12:07:10 UTC 2019

Our cron fired 6 times on startup - TriggerContext set to 60 seconds before "now" and CronTrigger set to fire every 10 seconds. After "catching up" scheduler will work and do it's job like a normal scheduled would do, apart from taking scheduled time when calculating next execution time instead of completion time, which is especially important in cases where you hit huge load and can't keep up with generating lets say these reports but once the load goes down, the reports will be processed and generated.

As for code:

I didn't write any tests. I'm not sure if changes didn't break some tests due to small edits in one or two interfaces (I tried to find all usages but I could miss some). I couldn't properly build project from source without excluding some tasks (-x test -x javadoc -x asciidoctor -x docsZip -x schemaZip -x distZip -x publishMavenJavaPublicationToMavenLocal). I tried to match the original code and style as closely as possible, I also provided some basic documentation for new things based on similar parts in classes. I needed this functionality and thought that making changes directly in Spring was the best way instead of writing custom logic, when most of it is already a part of framework. Adjust the code and add tests based on your preferences or needs.

@pivotal-issuemaster
Copy link

@MaciejGorczyca Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@MaciejGorczyca Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 26, 2019
@MaciejGorczyca
Copy link
Author

@sbrannen hey, could you please take a look at this? This pull request was left untouched for over half a year. I really believe it will be very helpful and it's rather simple and basic stuff that should be already in the Spring. Thanks!

@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 10, 2021
@snicoll
Copy link
Member

snicoll commented Sep 15, 2023

Thanks for the PR but TriggerContext is immutable on purpose and moving that update method to the interface is not something we're going to do.

Chatting with @jhoeller, it seems you want to restart trigger calculation from a custom last execution time. Rather than doing that via the TriggerContext you could specify the last execution time as an explicit argument or some kind of scheduling context.

The related issue is still open but didn't seem to get a lot of traction. If there's community interest, we'll get back to it.

@snicoll snicoll closed this Sep 15, 2023
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 15, 2023
@jhoeller
Copy link
Contributor

jhoeller commented Jan 4, 2024

FYI I'm addressing these requirements in CronTrigger itself rather than ReschedulingRunnable or TriggerContext-based methods: see #19475 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants