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

fix long period task will never be triggered #717

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

daydaychen
Copy link

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.

Copy link
Author

@daydaychen daydaychen left a 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.

@auvipy
Copy link
Member

auvipy commented Jan 19, 2024

The side effect is that adding a periodic task will trigger once first.

can you elaborate more on this please?

Copy link
Member

@auvipy auvipy left a 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

Copy link
Author

@daydaychen daydaychen left a 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.

Copy link
Author

@daydaychen daydaychen left a 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.

Copy link
Member

@auvipy auvipy left a 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

@auvipy
Copy link
Member

auvipy commented Jun 13, 2024

we need unit tests to make sure this won't create any regression/ unintended side effect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants