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(dlq): Added produce policy #57

Merged
merged 32 commits into from
Apr 29, 2022
Merged

Conversation

rahul-kumar-saini
Copy link
Contributor

I'm not sure if I did this correctly but eventually our DLQ policies need to produce invalid messages to a dead letter topic.

  • Introduced a new Policy which has the sole purpose of producing a given message to a dead letter topic
  • This could be extended by other policies such as the count policy to start putting the invalid messages into the dead letter topic instead of just ignoring them
  • Modified InvalidMessage Exception to also hold the original topic the message was produced to along with the timestamp the exception was thrown

@rahul-kumar-saini rahul-kumar-saini requested a review from a team as a code owner April 6, 2022 19:36
Copy link
Member

@evanh evanh left a 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.

@rahul-kumar-saini
Copy link
Contributor Author

rahul-kumar-saini commented Apr 8, 2022

A few questions I have before this is merged:

  • Since the exception can contain multiple messages, should they be produced individually? Currently it would just produce one message to the DLQ containing all of them.
  • Is there an easy way to test this policy? I'm not sure how to test that a message is produced to a topic so I haven't written any tests yet

Note:

  • I modified the count policy to inherit from this one, it seemed like the logical next step anyway - should be reviewed

Copy link
Member

@lynnagara lynnagara left a 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)?

@rahul-kumar-saini
Copy link
Contributor Author

@lynnagara
Yeah that was my question from my comment above your review, I'll change it to produce individually but I wasn't sure what to do.

@lynnagara
Copy link
Member

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:

  • Make the ignore and produce policies totally separate and making the user explicitly pick which one they want instead of passing a producer to CountInvalidMessagePolicy if they want it to produce. You could pass the limit to those other policies so they both still support the counting behavior.

  • Instead of passing the topic and producer to the count policy and making it inherit from ProduceInvalidMessagePolicy, how about passing a function instead, or even another policy class (that then does the producing or ignoring). The count policy would be more generic and could do anything the user wants after the limit is reached (not just ignoring or producing) and might be able to accomodate more custom behavior for different use cases.

What do you think?

@rahul-kumar-saini
Copy link
Contributor Author

@lynnagara

[...] or even another policy class (that then does the producing or ignoring). The count policy would be more generic and could do anything the user wants after the limit is reached (not just ignoring or producing) and might be able to accomodate more custom behavior for different use cases.

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.

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.

Getting closer, thanks

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 fix the to_dict method to avoid turning everything into strings.
Otherwise the rest is ok.

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 the comments Lyn added before merging

@rahul-kumar-saini rahul-kumar-saini merged commit b3f6867 into main Apr 29, 2022
@rahul-kumar-saini rahul-kumar-saini deleted the feat/produce-policy branch April 29, 2022 18:52
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

5 participants