Run until no more work is pending #134
Conversation
|
Big fan of this. This'd match my SQS polling where every x seconds I poll SQS and process whatever messages are in there, not just one. I can imagine that this can also be controlled by a boolean or an integer, representing the max number of tasks to process, to ship this as an opt-in behavior. |
|
Regarding whether this should be opt-in or opt-out... I see this quite clearly as a bug, and a fairly major one at that. I've used quite a few queueing systems, and they all basically attempt to do the work as fast as they can, they don't wait between jobs. Some provide a mechanism such as a refresh interval which is the time they will sleep before checking again for more work after having run out, but again that's not what we have here, even though I suspect that was the intention. Based on this, I'd suggest this should either be opt-out, or not given as a configuration option. If reviewers would like it to be a configuration option I'm not sure what it should even be called. That all said, this is based on my current understanding of the library, through the lens of the Fluent driver. It's still possible I've got this all wrong, and I'd be very happy to be shown my misunderstanding and an alternative way to fix the queues to run at full speed. |
|
I don't personally insist on having this opt out, was just thinking out loud. Happy to try to deploy this change to my backend to try to beta test this. |
|
@petrpavlik amazing, that would be really helpful if you could. I don't have any Swift in production (this is for a personal project) so while I can run this, check it does what I expect, etc, I'm not super confident in it. |
|
Yes, will do. I've already tried, the name of your work has a different name for some reason (vapor-queues vs.s queues), which would start yelling at me about some conflicts in my Package.swift. I'll try again, didn't have much time for it. |
Apologies. I didn't want a repo just named "queues" on my profile as it's not very descriptive out of the context of the Vapor org. Hopefully we can get these changes in soon anyway so it won't require a diff for too long. |
|
Thanks, I'll try to deploy it later today, worst case scenario I fork this repo myself and push the changes, so I don't have to deal with the rename-related issues. |
|
FWIW, when I switched to my fork it was a one line Package.swift change for the dependency. None of the code has been renamed, it's just the git repository URL which I assume would need to change anyway. Unless I've misunderstood the problem. |
|
Ok, I have this deployed now, seems to work fine so far. Will keep an eye on it. |
|
@0xTim @danpalmer ok, I've been running this for few weeks. Seems to work perfectly fine. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
==========================================
+ Coverage 82.86% 83.16% +0.29%
==========================================
Files 22 22
Lines 671 677 +6
==========================================
+ Hits 556 563 +7
+ Misses 115 114 -1
|
These changes are now available in 1.15.1
First pass at fixing #124.
Currently, every refresh interval, workers run a single task. This means that the maximum task throughput in the refresh interval is the number of workers.
With this change, every refresh interval, workers run tasks until there are no more tasks. This means that task throughput is tied to the speed of processing tasks, rather than just the number of workers.
Open to feedback about the approach, and I haven't added any tests here yet. This is a fairly major behavioural change, but should result in a large speed increase for most users.