experiment separate Broker + Router#624
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #624 +/- ##
==========================================
+ Coverage 79.79% 80.18% +0.39%
==========================================
Files 69 72 +3
Lines 2529 2771 +242
==========================================
+ Hits 2018 2222 +204
- Misses 511 549 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
s3rius
left a comment
s3rius
left a comment
There was a problem hiding this comment.
Well. Although the intention is good for sure.
But I see several problems with the solution you provided. Most importantly I see that brokers are referenced by strings almost everywhere. Which is problematic.
Also, since everything is implicit, it's hard to resolve conflicts in this setup. Like, what if I have multiple routers. I guess by design it's highly advised to only use one instance of router per project, but most probably people would not care about this constraint, since it's not enforced.
I'm not quite sure about Flow concept. Although multiple queue support is highly anticipated by the community, and I'm fully up for its support. But this implementation lacks some fundamental abilities for future expansion. Different brokers might have their own different parameters on Queues. For example RabbitMQ has lots of things that it can do with a queue.
And I'm not quite sure about flow_kind. It seems a bit redundant. Or I didn't catch the meaning of it.
I still like the intention. But I guess it need to be redesigned.
| ) | ||
|
|
||
| @classmethod | ||
| def queue(cls, name: str, **options: Any) -> "Flow": |
There was a problem hiding this comment.
I don't get what are odds in using different flow kinds? How are they going to differ?
|
|
||
| router.route_task( | ||
| send_email.task_name, | ||
| broker="priority", |
There was a problem hiding this comment.
Referencing by a magic string is not a good idea. I guess placing a broker value in here would be much better.
| try: | ||
| direct_result = await send_email(7, "welcome") | ||
|
|
||
| routed_task = await send_email.kiq(7, "welcome") |
There was a problem hiding this comment.
It says that the task is routed, but it's very implicit.
| routed_result = await routed_task.wait_result(timeout=2) | ||
|
|
||
| bulk_task = await send_email.kicker().with_route( | ||
| "default", |
2 participants
No description provided.