Add wrapper for Celery().send_task to support behavior as Task.apply_async#2377
Add wrapper for Celery().send_task to support behavior as Task.apply_async#2377sentrivana merged 18 commits intogetsentry:masterfrom
Celery().send_task to support behavior as Task.apply_async#2377Conversation
|
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? |
sentrivana
left a comment
There was a problem hiding this comment.
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
masterinto 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.
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.
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.
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.
I'll try to implement that and response if it'll be a lot slower than now
There was a problem hiding this comment.
@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.
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
|
sentrivana
left a comment
There was a problem hiding this comment.
Once CI on master is green, we can merge this
…r as `Task.apply_async` (getsentry#2377) --------- Co-authored-by: Vlad Vladov <vlad.vladov@alemira.com> Co-authored-by: Anton Pirker <anton.pirker@sentry.io> Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io> Co-authored-by: Ivana Kellyer <ivana.kellyer@sentry.io>
Close #2372
I've looked at code and decided to add the same wrapper to
send_taskas forapply_asyncbecause there is almost no difference besidesargsvariable.And inside test I checked if redis pipeline is worked with
send_taskandapply_asyncas well, but considering bug withtox --parallelI think there is some points to improve