-
-
Notifications
You must be signed in to change notification settings - Fork 7
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(dlq): Added produce policy #57
Conversation
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
A few questions I have before this is merged:
Note:
|
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that we'd be having a one-to-one relationship between the original messages and those produced on the dead letter topic. So if 2 of the original messages failed processing, even if they were processed by the strategy together, we'd end up with 2 messages on the dead letter topic - one for each. But this implementation is grouping them together into a single message. Any particular reason for the change (or was my original understanding incorrect)?
@lynnagara |
It's a bit unintuitive to me that CountInvalidMessagePolicy sort of flips between being an ignore policy and a produce policy depending on what arguments you pass to it. ProduceInvalidMessagePolicy is a bit like a CountInvalidMessagePolicy with a limit of 0 so I'm kind of unclear on what additional utility it provides. Some ideas that could help tackle this:
What do you think? |
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
I like this idea of chaining an additional policy to define whatever the user would like to happen with messages staying under the limit, I'll modify the policies to support it. |
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer, thanks
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the to_dict
method to avoid turning everything into strings.
Otherwise the rest is ok.
arroyo/processing/strategies/dead_letter_queue/policies/abstract.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
arroyo/processing/strategies/dead_letter_queue/policies/produce.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments Lyn added before merging
I'm not sure if I did this correctly but eventually our DLQ policies need to produce invalid messages to a dead letter topic.
InvalidMessage
Exception to also hold the original topic the message was produced to along with the timestamp the exception was thrown