Skip to content
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 support for processing more than 10 messages concurrently #271

Closed
wants to merge 9 commits into from

Conversation

schmod
Copy link

@schmod schmod commented Aug 25, 2021

Description

This change implements an internal work-queue inside of SQS-consumer using fastq.

This change enables SQS-consumer to process more than 10 messages concurrently, and also allows messages to be internally buffered.

Motivation and Context

This change enables or improves a few use-cases:

  • Queue workers can now process more than 10 messages at a time.
  • Queue workers no longer need to wait for an entire batch of messages to be processed before moving on to the next batch. This can significantly improve performance/efficiency in scenarios where processing-time varies between messages.

Additionally, by tracking work in a queue, it becomes possible to implement other features later on:

  • Graceful shutdowns (consumer.stop() can return a promise that resolves when the internal queue drains)
  • Exposing more detailed metrics (beyond isRunning) about the status of the consumer

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Resolves #96 and #229

@schmod schmod requested a review from a team as a code owner August 25, 2021 16:42
@schmod
Copy link
Author

schmod commented Aug 25, 2021

Apologies for opening a PR without first creating an issue. This is functionality that we need for an internal project, and we were going to build it one way or the other. Feel free to close this PR if this is not something that you'd like to adopt in the library. We'll gladly spin up our own fork if this is beyond BBC's desired scope of this library.

tomwinnington
tomwinnington previously approved these changes Aug 26, 2021
@nicholasgriffintn
Copy link
Member

These changes look to have been approved (new settings dismissed it), will need a triage and update due to its age though.

@nicholasgriffintn nicholasgriffintn requested a review from a team as a code owner December 9, 2022 17:50
@nicholasgriffintn
Copy link
Member

It looks like the previous suggestion was that this library should be limited to 10 messages at a time: #96 (comment)

I'm unsure if that's the best way of doing it though, I need to think about the advantages and disadvantages of doing this, in particular, I see we are using an additional library, which does add overhead that we'll need to support and ship, so some further consideration should be made.

@schmod
Copy link
Author

schmod commented Dec 16, 2022

I'll likely publish a fork if this is indeed a non-goal of sqs-consumer – the ability to buffer messages and process more than 10 at a time is unfortunately rather important for many workloads.

I appreciate the desire to keep this library as lightweight as possible, but I also wasn't able to identify a good pattern for building this functionality on top of sqs-consumer without modifying its internals. (If there was a way to expose an interface where this was possible, I'd support that.... but I'm not convinced that there is)

@nicholasgriffintn
Copy link
Member

I think given the age of this PR and the resolutions, it may be best to close this for now and look at this in a future version of sqs-consumer.

I have added an item on the Roadmap to look into this. Thanks for the contribution, but I'd like to fully understand our options here first as it's a big change to the library.

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process more than 10 messages in parallel
3 participants