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

Forward shadow option for retried tasks #6655

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

dsch
Copy link
Contributor

@dsch dsch commented Mar 2, 2021

Description

A retried task get logged with the task.name event if shadow is set.
This change sets the task shadow name also for retried tasks.

Current output

[INFO/MainProcess] Received task: MyTask[59e5c65f-544c-4cd4-8403-3975a802d0d5]
[INFO/MainProcess] Received task: tasks.add[59e5c65f-544c-4cd4-8403-3975a802d0d5] ETA:[2021-03-02 19:23:21.343560+00:00]
[INFO/ForkPoolWorker-8] Task MyTask[59e5c65f-544c-4cd4-8403-3975a802d0d5] retry: Retry in 10s
[INFO/ForkPoolWorker-8] Task tasks.add[59e5c65f-544c-4cd4-8403-3975a802d0d5] retry: Retry in 10s
[INFO/MainProcess] Received task: tasks.add[59e5c65f-544c-4cd4-8403-3975a802d0d5] ETA:[2021-03-02 19:23:33.023274+00:00]
[INFO/ForkPoolWorker-8] Task tasks.add[59e5c65f-544c-4cd4-8403-3975a802d0d5] retry: Retry in 10s
[INFO/MainProcess] Received task: tasks.add[59e5c65f-544c-4cd4-8403-3975a802d0d5] ETA:[2021-03-02 19:23:43.035171+00:00]

With this PR

[INFO/MainProcess] Received task: MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152]
[INFO/ForkPoolWorker-8] Task MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152] retry: Retry in 10s
[INFO/MainProcess] Received task: MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152] ETA:[2021-03-02 19:22:18.579569+00:00]
[INFO/ForkPoolWorker-8] Task MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152] retry: Retry in 10s
[INFO/MainProcess] Received task: MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152] ETA:[2021-03-02 19:22:29.610700+00:00]
[INFO/ForkPoolWorker-8] Task MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152] retry: Retry in 10s
[INFO/MainProcess] Received task: MyTask[9c1ecfea-d4a6-4cf6-ba21-073db73c0152] ETA:[2021-03-02 19:22:39.625028+00:00]

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #6655 (80de0cc) into master (07000d8) will decrease coverage by 5.18%.
The diff coverage is 39.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6655      +/-   ##
==========================================
- Coverage   75.60%   70.42%   -5.19%     
==========================================
  Files         138      138              
  Lines       16377    16447      +70     
  Branches     2052     2064      +12     
==========================================
- Hits        12382    11582     -800     
- Misses       3777     4662     +885     
+ Partials      218      203      -15     
Flag Coverage Δ
unittests 70.42% <39.86%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/app/control.py 91.36% <ø> (ø)
celery/app/defaults.py 97.33% <ø> (ø)
celery/backends/filesystem.py 96.42% <0.00%> (-3.58%) ⬇️
celery/bin/amqp.py 0.00% <0.00%> (ø)
celery/bin/base.py 0.00% <0.00%> (ø)
celery/bin/beat.py 0.00% <0.00%> (ø)
celery/bin/call.py 0.00% <0.00%> (ø)
celery/bin/celery.py 0.00% <0.00%> (ø)
celery/bin/control.py 0.00% <0.00%> (ø)
celery/bin/events.py 0.00% <0.00%> (ø)
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfa1b41...68c4314. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2021

This pull request introduces 1 alert and fixes 1 when merging 68c4314 into cfa1b41 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

auvipy
auvipy previously requested changes Mar 3, 2021
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.

Merging #6655 (80de0cc) into master (07000d8) will decrease coverage by 5.18%.
The diff coverage is 39.86%.

so we need more test coverage. 100% would great for this change

@thedrow thedrow added this to In progress in Celery 5.1.0 via automation Mar 4, 2021
@thedrow thedrow added this to the 5.1.0 milestone Mar 4, 2021
@thedrow
Copy link
Member

thedrow commented Mar 4, 2021

Unfortunately, I can't see the diff.
I'll clone the branch and check using diff-cover.

@thedrow
Copy link
Member

thedrow commented Mar 4, 2021

diff-cover coverage.xml --compare-branch master
-------------
Diff Coverage
Diff: master...HEAD, staged and unstaged changes
-------------
celery/app/task.py (100%)
-------------
Total:   1 line
Missing: 0 lines
Coverage: 100%
-------------

@thedrow thedrow dismissed auvipy’s stale review March 4, 2021 15:01

diff-cover says codecov is wrong and codecov doesn't provide a diff

Celery 5.1.0 automation moved this from In progress to Reviewer approved Mar 4, 2021
@thedrow thedrow merged commit bf5b2bc into celery:master Mar 4, 2021
Celery 5.1.0 automation moved this from Reviewer approved to Done Mar 4, 2021
jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
Co-authored-by: David Schneider <david.schneider@has-to-be.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Celery 5.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants