-
Notifications
You must be signed in to change notification settings - Fork 97
feat: add CRaC priming support to powertools-kafka module #2145
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: add CRaC priming support to powertools-kafka module #2145
Conversation
Hi @kjswaruph. Thank you for the PR! I have assigned the PR to you and we will begin reviewing it and giving feedback soon. |
Hey @kjswaruph, thanks for opening this PR 🚀 I will review it in depth next week. In the meantime, let's try to increase the priming to the other serializers beyond JSON. I think we can leverage Avro Let's ignore Protobuf for now since it doesn't support something similar to |
Hey @kjswaruph, do you already know when you can work on this? Let me know if you need any help! |
Hi @phipag, sorry for responding late. I’ll update the PR with the Avro GenericRecord as recommended and aim to finish it by this weekend. If I encounter any issues or need clarification, I’ll reach out. Thanks again for your feedback! |
2355544
to
46f66e3
Compare
@phipag I added the avro priming. Please review and tell me if any changes are required. |
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.
Hey @kjswaruph,
thanks again for updating the PR. I did an in-depth review of the code and tested the priming in my AWS account. I believe the Avro priming does not work because we need a type of SpecificRecordBase
and not GenericRecord
.
Due to the complexity of this (and increase in bundle size as a result) I am in favor of removing the Avro priming again and just keeping the JSON priming that you already included.
The rest looks good to me!
a9bb498
to
a91c090
Compare
- Add maven test profile and classesloaded.txt for preloading - Add Crac dependency and update PowertoolsSerializer to register as Crac Resource - Add tests in PowertoolsSerializerTest to assert beforeCheckpoint and afterRestore hooks do not throw exception
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
@phipag Sorry for the earlier confusion with the pr, I accidentally messed up the history while resolving merge conflicts. I have now cleaned up the branch and removed the Avro priming as requested. Please review again and let me know if there’s anything else needed. |
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.
Thanks @kjswaruph. This looks good to me. Can you please commit these two formatting fixes?
...rtools-kafka/src/main/java/software/amazon/lambda/powertools/kafka/PowertoolsSerializer.java
Outdated
Show resolved
Hide resolved
...rtools-kafka/src/main/java/software/amazon/lambda/powertools/kafka/PowertoolsSerializer.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Philipp Page <philipp.page@yahoo.de>
@phipag Done |
|
Summary
Add Crac support for kafka module of powertools.
Changes
Issue number: #1999
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.