refactor: replace the aimd algorithm (thanks Josh) with p-limit #107
Conversation
jeffsays
left a comment
jeffsays
left a comment
There was a problem hiding this comment.
this appears to be working on a queue with >19k items without immediately crashing or timing out
massfords
left a comment
massfords
left a comment
There was a problem hiding this comment.
This isn't ready to merge yet. I think your local branch has additional changes that got us through the re-drive.
The static config of p-limit instead of the reservedConcurrency or a param isn't a good general purpose solution.
Reverting to the aimd is also in play now that there's time to dig through this a little more to see why it flooded the client with the TimeoutError immediately after starting.
| // todo - can get the limit from the lambda's reserved concurrency, but 10 is a reasonable default | ||
| const limit = pLimit(10); |
There was a problem hiding this comment.
I don't think we should merge this with this todo not done. The reserved concurrency should be available in the function config fetched above.
| promises.push( | ||
| // eslint-disable-next-line no-loop-func | ||
| control(async (w: number) => { | ||
| limit(async () => { |
There was a problem hiding this comment.
@jeffsays I think your local branch had another change where the array of promises was awaited when it got too big. If we're going to maintain this tool, that should be merged into this since this solution still tipped over when there were hundreds of thousands of messages being re-driven.
Summary
What does this PR do?