-
Notifications
You must be signed in to change notification settings - Fork 205
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
Store current time when TaskResult started #432
Conversation
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.
I like the intent of the PR. however I need more through review before merge it
in the mean time, can you please review this PR? #407 |
@auvipy Thank you for your quick review. I have confirmed the fix for #407. In my personal opinion, |
@@ -61,8 +61,8 @@ To use :pypi:`django-celery-results` with your project you need to follow these | |||
|
|||
CELERY_RESULT_EXTENDED = True | |||
|
|||
If you want to track the execution duration of your tasks (by comparing `date_created` and `date_done` in TaskResult), enable the :setting:`track_started` setting. | |||
|
|||
If you want to track the execution duration of your tasks (by comparing `date_started` and `date_done` in TaskResult), enable the :setting:`track_started` setting. |
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.
can we think of any example, using the new feature here?
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.
@auvipy Added example. However, I'm not good at English, so I would like you to correct the sentence.
By the way, you previously asked whether this package should include the behavior of registering TaskResult at states.PENDING
timing.
ref: #286 (comment)
If it is possible for me, I would like to create another PR. What do you think?
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.
you can certainly open another PR. but what are the issues with the open PR?
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.
Sorry I'm not good at explaining. There is nothing problems with this PR.
I would like to submit a new PR for the PENDING status registration.
If there is no problem with this PR, could you please merge it?
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.
you are most welcome to come with a new PR
Thank you to all maintainers of this great OSS.
Referring to the issue below, I generate TaskResult when Task is
PENDING
.ref: #286 (comment)
This code works fine, but I can't use
date_created
as the start time of the Task because the TaskResult already exists whenSTARTED
is recorded.Additionally, in my use case, I want to measure the time from when a task is added to the message broker until the worker starts processing it.
By recording
date_started
as the time associated withSTARTED
, we can calculate the waiting time in the queue or the actual processing time of the worker.Use
django.db.models.functions.Now
to get the current time to avoid being affected by the server's time settings.ref: https://docs.djangoproject.com/en/5.0/ref/models/database-functions/#now
This PR is backward compatible because it simply adds a column and sets a value.
For users who do not create TaskResults themselves,
date_created
anddate_started
have the same meaning.Testing
We can check the value of
date_started
in django admin.For testing, I implemented a 5 second sleep on
@shared_task
.The time increases in the order
date_created < date_started < date_done
.Please let me know if there is anything else that should be tested.