Clear old jobs while loading the jobs from schedule #405

sandip-mane · 2023-03-03T05:27:31Z

Patch for #396

sandip-mane · 2023-03-03T05:28:59Z

@markets I was not able to quickly update PR #396, hence created a new one. Please review.

markets · 2023-03-03T08:54:57Z

Thanks @sandip-mane! Maybe we should update some tests too? I think the related tests are still using the non-bang version.

markets · 2023-03-03T09:26:46Z

test/unit/job_test.rb

+        out = Sidekiq::Cron::Job.load_from_hash! @jobs_hash
        assert_equal out.size, 0, "should have no errors"
        assert_equal Sidekiq::Cron::Job.all.size, 2, "Should have 2 jobs after load"

-        out_2 = Sidekiq::Cron::Job.load_from_hash @jobs_hash
+        out_2 = Sidekiq::Cron::Job.load_from_hash! @jobs_hash


I was referring the ScheduleLoader tests: https://github.com/sidekiq-cron/sidekiq-cron/blob/master/test/unit/schedule_loader_test.rb. Not sure if those should be updated too 🤔.

Yes, seemed like the right thing to do. Have updated the tests.

codecov · 2023-03-03T10:50:19Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e01b653). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #405   +/-   ##
=========================================
  Coverage          ?   93.36%           
=========================================
  Files             ?       10           
  Lines             ?      482           
  Branches          ?        0           
=========================================
  Hits              ?      450           
  Misses            ?       32           
  Partials          ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fabn · 2023-05-25T13:32:29Z

This should have been a breaking change, I used to create some cronjobs programmatically and they are gone on every restart.

rchasman

This PR introduced a massive issue for us, where it destroys existing jobs when we deploy.
We only load some of our schedules from a YAML, because of this, the ones that are dynamically created and stored in Redis are cleared.

sandip-mane · 2023-10-20T06:36:10Z

@markets shall we revert this change? The issue with dynamic cron jobs will continue to be an issue on every deploy IMO.
But it also leads to another issue where if I modify the class for an existing cron job, then I have to manually delete the old one using a script.

I think the best way would be to document the usage of dynamic cron jobs.

Thoughts?

sandip-mane · 2023-10-20T06:39:37Z

We only load some of our schedules from a YAML, because of this, the ones that are dynamically created and stored in Redis are cleared.

@rchasman are all jobs from ScheduledSet cleared or only the Cron jobs are cleared?

markets · 2023-10-20T11:19:26Z

hmm 🤔 maybe we should revert this change... It seems a good idea, but it seems it breaks with dynamic jobs, which is worst scenario compared to old jobs in the schedule (that can be cleaned manually).

Another idea (probably ideal solution?) is to only cleanup the ones defined in the YAML, but right now, we don't have a mechanism (an attribute or an extra data structure) to distinct between dynamic jobs vs static (defined in the YAML).

So yes, maybe it's better to revert it and document it?

sandip-mane · 2023-10-20T11:30:07Z

My honest opinion:
Cron schedule should be kept up to date upon every deployment. (For various reasons, for example, I rename an existing cronjob class, delete a cron-job etc)
This creates an issue with dynamic cron jobs, so they need to be handled separately. I have not worked on these yet so not sure what could be the best way to deal with them.
Probably having an attribute for dynamic jobs could help distinguish between the jobs and we will be able to handle it from the gem.

I feel this should be the plan:

Revert & release
Have a way to distinguish dynamic crons
Add this change back (it should only clear non-dynamic crons)

markets · 2023-10-20T11:34:23Z

Agree with the plan! Please, feel free to send PRs to move this forward, unfortunately I've very limited time at this point to work on these changes.

…cron#405)" This reverts commit 48c2563.

…427) This reverts commit 48c2563.

markets · 2023-10-23T18:00:25Z

hey @sandip-mane 👋🏼 I already merged the revert, but before pushing a new release I think it would be better to have the "dynamic crons" thing (points 2 and 3 of the plan). This way, we'll publish a much more solid release. If we release only the revert, the original issues may arise again... What do you think?

sandip-mane · 2023-10-26T15:03:13Z

Sure sure, I will try to send PRs for the other 2 issues by this weekend (Working on a release towards a deadline).

Clear old jobs while loading the jobs from schedule

b98f25b

sandip-mane mentioned this pull request Mar 3, 2023

Destroy all scheduled jobs before load a new on startup #396

Closed

Updates the test case for the change

a1f980b

markets reviewed Mar 3, 2023

View reviewed changes

Updated the schedule loader tests

0450802

markets approved these changes Mar 3, 2023

View reviewed changes

markets merged commit 48c2563 into sidekiq-cron:master Mar 3, 2023

markets mentioned this pull request Mar 8, 2023

Multiple jobs (Rails mailers) scheduled at the same time #400

Closed

skelz0r mentioned this pull request Aug 30, 2023

BUGFIX Sidekiq cron loading etalab/admin_api_entreprise#1196

Merged

rchasman reviewed Oct 19, 2023

View reviewed changes

sandip-mane added a commit to sandip-mane/sidekiq-cron that referenced this pull request Oct 20, 2023

Revert "Clear old jobs while loading the jobs from schedule (sidekiq-…

b06bc5a

…cron#405)" This reverts commit 48c2563.

This was referenced Oct 20, 2023

Add ability to differentiate between regular cron jobs and dynamic cron jobs #428

Closed

Revert changes that clear the jobs upon loading schedule #426

Closed

Clear non-dynamic cron jobs upon loading the schedule #429

Closed

markets pushed a commit that referenced this pull request Oct 20, 2023

Revert "Clear old jobs while loading the jobs from schedule (#405)" (#…

6b00fa0

…427) This reverts commit 48c2563.

markets mentioned this pull request May 7, 2024

load_from_array! showing "Cron Jobs - added job with name ..." for previously added jobs #467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear old jobs while loading the jobs from schedule #405

Clear old jobs while loading the jobs from schedule #405

sandip-mane commented Mar 3, 2023

sandip-mane commented Mar 3, 2023

markets commented Mar 3, 2023

markets Mar 3, 2023 •

edited

Loading

sandip-mane Mar 3, 2023

codecov bot commented Mar 3, 2023

fabn commented May 25, 2023

rchasman left a comment •

edited

Loading

sandip-mane commented Oct 20, 2023

sandip-mane commented Oct 20, 2023

markets commented Oct 20, 2023

sandip-mane commented Oct 20, 2023

markets commented Oct 20, 2023

markets commented Oct 23, 2023

sandip-mane commented Oct 26, 2023

Clear old jobs while loading the jobs from schedule #405

Clear old jobs while loading the jobs from schedule #405

Conversation

sandip-mane commented Mar 3, 2023

sandip-mane commented Mar 3, 2023

markets commented Mar 3, 2023

markets Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

sandip-mane Mar 3, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2023

Codecov Report

fabn commented May 25, 2023

rchasman left a comment • edited Loading

Choose a reason for hiding this comment

sandip-mane commented Oct 20, 2023

sandip-mane commented Oct 20, 2023

markets commented Oct 20, 2023

sandip-mane commented Oct 20, 2023

markets commented Oct 20, 2023

markets commented Oct 23, 2023

sandip-mane commented Oct 26, 2023

markets Mar 3, 2023 •

edited

Loading

rchasman left a comment •

edited

Loading