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

Route chord_unlock task to the same queue as chord body #6896

Conversation

MelnykR
Copy link
Contributor

@MelnykR MelnykR commented Aug 5, 2021

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

Fixes #6888

Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just the fix I had in mind so well done!
Could you please include a unit test in this patch to verify that this works as expected?

@thedrow
Copy link
Member

thedrow commented Aug 5, 2021

The relevant test fails as the expected outcome has changed.
We need to adjust it before we merge this PR.

@thedrow thedrow marked this pull request as draft August 5, 2021 16:57
@MelnykR
Copy link
Contributor Author

MelnykR commented Aug 5, 2021

This was just the fix I had in mind so well done!

Glad to hear that! That was the intention of creating issue and PR -- get feedback whether I am on the right path.

The relevant test fails as the expected outcome has changed.
We need to adjust it before we merge this PR.

I guess I fixed existing tests. But I think new test should be also added. I will try to work on this on the weekend.

Thanks for the feedback!

@lgtm-com
Copy link

lgtm-com bot commented Aug 6, 2021

This pull request introduces 2 alerts when merging 4d317f1 into 846066a - view on LGTM.com

new alerts:

  • 2 for Wrong name for an argument in a call

@codecov
Copy link

codecov bot commented Aug 15, 2021

Codecov Report

Merging #6896 (4d317f1) into master (1c477c4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6896   +/-   ##
=======================================
  Coverage   89.23%   89.23%           
=======================================
  Files         138      138           
  Lines       16626    16628    +2     
  Branches     2099     2100    +1     
=======================================
+ Hits        14836    14838    +2     
  Misses       1571     1571           
  Partials      219      219           
Flag Coverage Δ
unittests 89.22% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
celery/backends/base.py 93.42% <100.00%> (+0.02%) ⬆️

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 186fa47...4d317f1. Read the comment docs.

@auvipy auvipy marked this pull request as ready for review August 15, 2021 17:16
@auvipy auvipy merged commit 38a645d into celery:master Aug 15, 2021
@auvipy
Copy link
Member

auvipy commented Aug 15, 2021

thanks!

jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
* Route chord_unlock task to the same queue as chord body

* fix existing  tests

* add case to cover bugfix
@MelnykR MelnykR deleted the bugfix/6888-route_chord_unlock_to_the_same_queue_as_chord_body branch November 16, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chord_unlock isn't using the same queue as chord body task
3 participants