Fix update tasks #3958

brianmay · 2017-04-05T04:40:53Z

#3791 was improved and become #3890. This is an improvement on #3890, as it addresses some of the feedback supplied there.

Checks if the heap is valid. If there are changes in scheduled tasks the heap is populated again.

This means, when using django-celery-beat, changes to the periodictask table are automatically detected without restarting celery.

Fixes celery/django-celery-beat#7

I can squash/rebase/split/reformat/turn-into-a-penguin as required.

brianmay · 2017-04-05T05:14:49Z

Just updated this to fix some of the problems I encountered. Should work now regardless of how you change the schedule.

brianmay · 2017-04-05T05:40:39Z

celery/beat.py

+            b_model = b.get(name)
+            if not b_model:
+                return False
+            if model.schedule != b_model.schedule:


This code looks like it is specific the Django DB models used in django_celery_beat. May not be appropriate to include in celery???

Oh, wait, my objection to my own pull request might be unfounded.

django_celery_beat.schedulers.ModelEntry is actually inherited from celery.beat ScheduleEntry - so I am guessing is actually a standard celery object with some extra bits added.

I believe the above code is fine.

For the record, on this line we are comparing two celery.schedules.schedule objects (for interval times) or two celery.schedules.crontab objects (for crontab entries). Or maybe one of each (if the user has just changed from one to the other).

thedrow

Minor comments.
Also, can we add tests for schedules_equal?

thedrow · 2017-04-08T09:13:29Z

celery/beat.py

+    def schedules_equal(self, a, b):
+        if set(a.keys()) != set(b.keys()):
+            return False
+        for name, model in a.items():


I think entry is a better name than model. Models are specific to ORMs.

Yes, agreed. Probably partly responsible for my confusion above.

thedrow · 2017-04-08T09:16:14Z

celery/beat.py

+            return False
+        for name, model in a.items():
+            b_model = b.get(name)
+            if not b_model:


There's no need for a separate if statement here. Use or instead.

thedrow · 2017-04-08T09:17:54Z

celery/beat.py

@@ -281,6 +284,17 @@ def tick(self, event_t=event_t, min=min, heappop=heapq.heappop,
                return min(verify[0], max_interval)
        return min(adjust(next_time_to_run) or max_interval, max_interval)

+    def schedules_equal(self, a, b):


Let's use more meaningful names here. old_schedule and new_schedule works imo.

brianmay · 2017-04-09T23:29:18Z

Have addressed minor issues. Will see if I can do anything about tests.

brianmay · 2017-04-10T00:28:20Z

In writing a test, I found that when we assign schedule to old_schedule we copy the reference to the dictionary. So if you mutate the dictionary - as happens by default with the methods provided - you compare the dictionary with itself. I have now changed that to do a shallow copy of the dictionary.

Otherwise, if we mutate the dictionary (e.g. using provided methods), we won't see any changes.

brianmay · 2017-04-10T00:37:01Z

Ok, just pushed tests. Have confirmed that the test to check the schedule change works breaks without this change.

brianmay · 2017-04-10T00:49:51Z

In doing a shallow copy, I am making the assumption that that the ScheduleEntry objects (and nested schedule objects) will not get mutated. Which is probably correct, but can't be guaranteed, especially as the ScheduleEntry object has an update() method, designed to mutate its state.

I tried using copy.deepcopy also, but this doesn't work here.

brianmay · 2017-04-10T00:51:08Z

It looks like travis crashed before it even got to running the tests.

georgepsarakis · 2017-04-10T04:56:48Z

@brianmay I restarted the failed job.

thedrow · 2017-04-10T06:48:48Z

Thanks. Feel free to add yourself to the AUTHORS file.

See celery#3958.

See #3958.

evasilchenko · 2017-04-17T16:33:54Z

Are there plans to get this fix into an official new release soon?

thedrow · 2017-04-17T17:20:28Z

Yes. It will land on the next release.

brianmay · 2017-05-16T23:14:38Z

When is the next release due? Are there any obstacles in the way of a release?

thedrow · 2017-05-28T04:19:50Z

We're working on it. Right now debating about a merged pull request with possible security issues. See celery/py-amqp#146 (comment)

Gurubol · 2017-06-11T05:06:17Z

@thedrow , Looks like the security issue you were talking about is closed. Can we please know when will this fix be released? Thanks.

thedrow · 2017-06-11T12:30:29Z

I'm trying to reach @ask to provide us with release access. I'll let you know once it happens.

See celery#3958.

elyssonmr · 2017-07-10T01:10:36Z

Hello guys!
I'm trying to build a "task agenda" and I'm going to need this fix... there is any release date planned?
I saw last comment dated from 29 days ago (when I wrote this comment).

Thanks :D

thedrow · 2017-07-10T04:11:07Z

We're unable to reach the maintainer with the release rights. I don't have an ETA for the release.

brianmay · 2017-07-10T04:49:57Z

@thedrow I assume you mean you can't upload to PyPI? If so, maybe contact PyPI admin and see if they can help? In the meantime maybe you could do the other steps of a release, e.g. use bumpversion to increment the version and add a git tag. Thanks.

mheppner · 2017-07-10T13:52:53Z

@thedrow Slightly related, but #4100 could use some feedback on who the project maintainers are and who has release rights.

sreecodeslayer · 2017-11-08T05:43:33Z

I want this one released so badly ☹️

thedrow · 2017-11-08T13:47:48Z

@sreecodeslayer This was released as part of 4.1.0 a while ago.

Gurubol · 2017-11-08T14:00:50Z

@thedrow , I see that the build has failed. Please check status at https://travis-ci.org/celery/celery/jobs/299003848 for the build at https://travis-ci.org/celery/celery

Can you please clarify?

thedrow · 2017-11-09T07:23:19Z

We're aware that the build has failed.
We'll look into it once we have the time to do so.

brianmay · 2017-11-12T22:12:09Z

In my latest testing, it seems that I still have issues with the schedule not being reloaded in master :-(
That is when I modify a crontab database entry, I have to restart beat for the changes to have affect. Will do some more testing.

brianmay · 2017-11-12T22:21:16Z

Just set a task to run every 2 minutes. It runs the first time, but then fails to run again until restarting celery. This is with master. Will try downgrading to the most recent stable version and see if that helps.

brianmay · 2017-11-12T22:34:30Z

Me confused, celery 4.1.0 doesn't seem to be running interval tasks at all. Was working last time I tested it. Will concentrate my efforts on master.

brianmay · 2017-11-12T22:50:47Z

In master, changing the periodic task to point to a different interval or crontab object results in an instant change, but changing the interval object or crontab itself doesn't. Curious, as this is a case a tested in the pull request. Also the task - if it is an interval - only gets run once. Also curious.

brianmay · 2017-11-12T23:21:16Z

Oops, just realised a new version of django-celery-beat (1.1.0) was released 13 days ago. Apologies for the noise.

The schedule now reloads as required. However I still have this issue where a interval based task only runs once (after restarting task or updating the schedule).

thedrow · 2017-11-13T13:37:10Z

@brianmay Please open a new issue if one doesn't exist for the problem you are describing.

brianmay · 2017-11-13T20:00:53Z

@thedrow, ok will do so.

CharmingYang0 · 2017-11-15T16:43:28Z

@brianmay
Hopefully, this fix will be involved in celery 4.1.1 and this version is released soon.

See celery#3958.

Populate heap when periodic tasks are changed

92cd811

brianmay force-pushed the fix_update_tasks branch from a527662 to dcf7eb9 Compare April 5, 2017 05:12

brianmay mentioned this pull request Apr 5, 2017

Populate heap when periodic tasks are changed - fixed PEP8 #3890

Closed

brianmay commented Apr 5, 2017

View reviewed changes

brianmay mentioned this pull request Apr 5, 2017

Django Celery Beat admin updating Cron Schedule Periodic task not taking effect? celery/django-celery-beat#7

Closed

thedrow requested changes Apr 8, 2017

View reviewed changes

brianmay added 6 commits April 10, 2017 09:18

Address feedback from previous commit

9582643

Rename a to old_schedules

f633297

Rename b to new_schedules

a0fc880

Rename model to old_entry

ecd2d56

Rename b to new_entry

4e8764e

Simplify if condition using or

e0f4f60

brianmay force-pushed the fix_update_tasks branch from dcf7eb9 to e0f4f60 Compare April 9, 2017 23:26

brianmay added 2 commits April 10, 2017 10:29

Make shallow copy of schedules dictionary

f201506

Otherwise, if we mutate the dictionary (e.g. using provided methods), we won't see any changes.

Add tests for ensuring schedule changes are detected

6fa34bc

thedrow merged commit b27c0f1 into celery:master Apr 10, 2017

brianmay deleted the fix_update_tasks branch April 10, 2017 23:03

brianmay added a commit to brianmay/celery that referenced this pull request Apr 10, 2017

Add self to CONTRIBUTORS.txt

5c15811

See celery#3958.

This was referenced Apr 10, 2017

Add self to CONTRIBUTORS.txt #3970

Merged

Populate heap when periodic tasks are changed #3791

Closed

georgepsarakis pushed a commit that referenced this pull request Apr 11, 2017

Add self to CONTRIBUTORS.txt

2552c59

See #3958.

aatishnn mentioned this pull request Apr 13, 2017

celery beat can not change the task config at runtime #3901

Closed

thedrow pushed a commit to staticfox/celery that referenced this pull request Jul 6, 2017

Add self to CONTRIBUTORS.txt

ecd4590

See celery#3958.

brianmay mentioned this pull request Jul 18, 2017

Memory addresses being output that break reproducible builds sphinx-doc/sphinx#3722

Closed

thedrow added the Component: Celerybeat label Oct 16, 2017

ntcong mentioned this pull request Nov 2, 2017

How to dynamically add or remove tasks to celerybeat? #3493

Closed

brianmay mentioned this pull request Nov 15, 2017

beaats interval tasks only getting run once #4384

Closed

auvipy added this to the v4.2 milestone Jan 17, 2018

SebastianBerchtold pushed a commit to smartlane/celery that referenced this pull request Jun 11, 2018

Add self to CONTRIBUTORS.txt

cea4fee

See celery#3958.

vilhelmen mentioned this pull request Jul 26, 2023

Non-recurring future tasks #8394

Open

4 tasks

Fix update tasks #3958

Fix update tasks #3958

Conversation

brianmay commented Apr 5, 2017 • edited

brianmay commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianmay commented Apr 9, 2017

brianmay commented Apr 10, 2017 • edited

brianmay commented Apr 10, 2017

brianmay commented Apr 10, 2017 • edited

brianmay commented Apr 10, 2017

georgepsarakis commented Apr 10, 2017

thedrow commented Apr 10, 2017

evasilchenko commented Apr 17, 2017

thedrow commented Apr 17, 2017

brianmay commented May 16, 2017

thedrow commented May 28, 2017

Gurubol commented Jun 11, 2017

thedrow commented Jun 11, 2017

elyssonmr commented Jul 10, 2017

thedrow commented Jul 10, 2017

brianmay commented Jul 10, 2017

mheppner commented Jul 10, 2017

sreecodeslayer commented Nov 8, 2017

thedrow commented Nov 8, 2017

Gurubol commented Nov 8, 2017 • edited

thedrow commented Nov 9, 2017

brianmay commented Nov 12, 2017

brianmay commented Nov 12, 2017 • edited

brianmay commented Nov 12, 2017

brianmay commented Nov 12, 2017 • edited

brianmay commented Nov 12, 2017 • edited

thedrow commented Nov 13, 2017

brianmay commented Nov 13, 2017

CharmingYang0 commented Nov 15, 2017

brianmay commented Apr 5, 2017 •

edited

brianmay commented Apr 10, 2017 •

edited

brianmay commented Apr 10, 2017 •

edited

Gurubol commented Nov 8, 2017 •

edited

brianmay commented Nov 12, 2017 •

edited

brianmay commented Nov 12, 2017 •

edited

brianmay commented Nov 12, 2017 •

edited