-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix: Handle batch failures in FIFO queues correctly #1183
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #1183 +/- ##
============================================
+ Coverage 78.94% 79.08% +0.13%
- Complexity 629 634 +5
============================================
Files 72 73 +1
Lines 2332 2352 +20
Branches 253 256 +3
============================================
+ Hits 1841 1860 +19
Misses 412 412
- Partials 79 80 +1
☔ View full report in Codecov by Sentry. |
Hey @mriccia do you have time to look at this? Trying to spread the PR review burden around ;) |
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.
Tests are not easy to read, but probably because the code itself is... Few comments, Would be great to have another pair of eyes on it.
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsUtils.java
Outdated
Show resolved
Hide resolved
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsUtils.java
Outdated
Show resolved
Hide resolved
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsUtils.java
Outdated
Show resolved
Hide resolved
...software/amazon/lambda/powertools/sqs/exception/SkippedMessageDueToFailedBatchException.java
Outdated
Show resolved
Hide resolved
...-sqs/src/test/java/software/amazon/lambda/powertools/sqs/SqsUtilsFifoBatchProcessorTest.java
Outdated
Show resolved
Hide resolved
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsUtils.java
Outdated
Show resolved
Hide resolved
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsUtils.java
Outdated
Show resolved
Hide resolved
...-sqs/src/test/java/software/amazon/lambda/powertools/sqs/SqsUtilsFifoBatchProcessorTest.java
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.
Almost there
...-sqs/src/test/java/software/amazon/lambda/powertools/sqs/SqsUtilsFifoBatchProcessorTest.java
Outdated
Show resolved
Hide resolved
…/sqs/SqsUtils.java Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com>
…ure' into fix-fifo-batch-failure
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsUtils.java
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.
just remove the break and we're good
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.
LGTM !!
Issue #, if available: #1140
Description of changes:
When processing a FIFO-queue batch, once a message for a given
messageGroupId
fails, no subsequent messages in the group should be processed. This ensures ordering of messages is maintained.The implementation for powertools python is here, and the documentation here. Note that the python implementation requires the user to specify the FIFO batch processor explicitly. I believe this is unnecessary - we can tell that we are working with a FIFO queue when we see the
MessageGroupId
attribute on the message.This implementation is related to #797 which will see us redo our batch processing implementation. IMHO it is worth fixing our existing bug with the existing FIFO implementation as the new version that supersedes this will likely be an opt-in breaking change and is a much more significant piece of work.
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.