-
Notifications
You must be signed in to change notification settings - Fork 514
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
Add wrapper for Celery().send_task
to support behavior as Task.apply_async
#2377
Conversation
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Any review? |
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.
Hey @divaltor, thanks for the PR and sorry for taking so long to review this. The change looks good to me in general. Could you additionally:
- rebase on/merge current
master
into your branch, there's a conflict that needs resolving - address the comments I left on the PR
Thanks again! If you don't have the time we're also happy to take it from here and push this over the finish line.
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.
It'd be great to test the send_task
wrapping more thoroughly with more test cases rather than just the one -- can you add it to the celery_invocation
fixture?
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 problem with passing Celery
instance to that fixture is that we need to create whole worker with some "real" one backend like Redis, because Celery.send_task
ignores configuration for eager tasks
I think it possible to add that to celery_invocation
, but it'll increase tests duration because we need to create "real" instance almost each run
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.
Gotcha. Do we know how much it would slow down the tests? I'd definitely prefer if we could run all of the existing tests where we test apply_async
with send_task
as well since that way we'd be able to catch regressions if there's a change in Celery upstream that only affects send_task
but not apply_async
. Unless this makes the tests horrifically slow.
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'll try to implement that and response if it'll be a lot slower than now
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.
@sentrivana Sorry for bothering you. Tried to refactor that thing, but it took more time and effort than it was look. Unfortunately, I don't have much time to make that thing done, so I'm really feel sorry that I can't complete it right now
If you want - you can take this branch as is and work on it, if you think you have resources for that, thanks for understanding 😞
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.
Hey @divaltor, thanks a ton for your work on this so far! I understand of course -- no worries, we can take it from here. :)
7d19bdd
to
016609c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2377 +/- ##
==========================================
- Coverage 79.44% 78.44% -1.00%
==========================================
Files 136 136
Lines 14641 14649 +8
Branches 3076 3078 +2
==========================================
- Hits 11632 11492 -140
- Misses 2170 2329 +159
+ Partials 839 828 -11
|
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.
Once CI on master is green, we can merge this
…r as `Task.apply_async` (getsentry#2377) --------- Co-authored-by: Vlad Vladov <[email protected]> Co-authored-by: Anton Pirker <[email protected]> Co-authored-by: Daniel Szoke <[email protected]> Co-authored-by: Ivana Kellyer <[email protected]>
Close #2372
I've looked at code and decided to add the same wrapper to
send_task
as forapply_async
because there is almost no difference besidesargs
variable.And inside test I checked if redis pipeline is worked with
send_task
andapply_async
as well, but considering bug withtox --parallel
I think there is some points to improve