-
Notifications
You must be signed in to change notification settings - Fork 429
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
fix long period task will never be triggered #717
base: main
Are you sure you want to change the base?
Conversation
64570cc
to
28ea74e
Compare
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.
The side effect is that adding a periodic task will trigger once first.
can you elaborate more on this please? |
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.
we need proper unit test for the change to make sure there won't be any regression
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.
The side effect is that adding a periodic task will trigger once first.
can you elaborate more on this please?
When the last_run_at
value is None
, it defaults to the reload time of the entry.
For example, the first trigger time of a three-day interval scheduled task should be last_run_at
+3 days, but each time the schedule_changed
method return True
, the entry will be re-instantiated and the last_run_at
will be updated.
This can be a problem for the schedule_changed
method frequently triggered by the management panel, because the last_run_at
method will be frequently refreshed and may never be executed as planned.
Sorry, there were some side effects for the previous implementation.
I made some changes to the initial value of last_run_at
.
Now, if it's None
, it will be fetched from date_changed
.
In this way, the default value of last_run_at
will not affected by the schedule_changed
, the above problems can be solved.
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.
we need proper unit test for the change to make sure there won't be any regression
Ok, I'll make up the unit test later.
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.
waiting for unit tests to make sure this will not create any regression
we need unit tests to make sure this won't create any regression/ unintended side effect |
Here, the entry reload time is taken as the default value when last_run_at is None,
and this value will be updated every time the schedule is updated/added.
For a periodic task with a long period, such as three days, if there is always an update/add within three days, the periodic task will never be triggered.