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

feat(consumer): Wrap consumer strategy with DLQ if it exists #56

Merged
merged 3 commits into from
Apr 7, 2022

Conversation

rahul-kumar-saini
Copy link
Contributor

@rahul-kumar-saini rahul-kumar-saini commented Apr 6, 2022

  • ConsumerStrategyFactory builds the main strategy used by all consumers in Snuba
  • Optionally pass a DLQ policy to the Consumer Strategy Factory
  • This will wrap the strategy with a DLQ with the given policy

Here's an example of how this will be used:
getsentry/snuba#2585

@onewland
Copy link

onewland commented Apr 6, 2022

Looks good to me though I think it should get a look from somebody who's more arroyo-familiar


The `dead_letter_queue_policy` defines what to do when an bad message
is encountered throughout the next processing step(s). A DLQ wraps the
entire strategy and handles a specific exception to handle bad messages.
Copy link
Member

@MeredithAnya MeredithAnya Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the second sentence is a little confusing to me, would it be fair to say something like?:

A DLQ policy wraps the entire strategy, catching InvalidMessage exceptions and handling them as the policy dictates.

Copy link
Contributor

@fpacifici fpacifici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Meredith's comment, otherwise it is ok.

@rahul-kumar-saini rahul-kumar-saini merged commit 74f3cf0 into main Apr 7, 2022
@rahul-kumar-saini rahul-kumar-saini deleted the feat/dlq-factory-wrapper branch April 7, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants