feat(batch-processing): move non retry-able message to DLQ #500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I need to further strengthen this with test cases. If someone wants to give an early pair of 👀 |
pankajagrawal16
commented
Aug 12, 2021
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsBatch.java
Show resolved
Hide resolved
pankajagrawal16
commented
Aug 12, 2021
powertools-sqs/src/main/java/software/amazon/lambda/powertools/sqs/SqsBatch.java
Show resolved
Hide resolved
pankajagrawal16
force-pushed
the
sqs-dlq-non-retryable
branch
from
7520d42
to
b494fa2
Compare
pankajagrawal16
changed the title
Updated tests now. I plan to take doc update as a separate PR |
pankajagrawal16
force-pushed
the
sqs-dlq-non-retryable
branch
2 times, most recently
from
0b036d9
to
9c696f5
Compare
msailes
reviewed
Aug 24, 2021
| @@ -32,6 +32,19 @@ | |||
| * {@link SqsBatch#suppressException()} to true. By default its value is false | |||
| * </p> | |||
| * | |||
| * <p> | |||
| * If you want certain exceptions to be treated as permanent failures, i.e. exceptions which are not worth retrying and | |||
There was a problem hiding this comment.
I think we should change the phrase "not worth retying" to something like "exceptions where the result of retrying will always be a failure"
msailes
reviewed
Aug 24, 2021
| @@ -32,6 +32,19 @@ | |||
| * {@link SqsBatch#suppressException()} to true. By default its value is false | |||
| * </p> | |||
| * | |||
| * <p> | |||
| * If you want certain exceptions to be treated as permanent failures, i.e. exceptions which are not worth retrying and | |||
| * want such message should be moved to configured dead letter queue of the source SQS queue, you can use | |||
There was a problem hiding this comment.
"these can be immediately moved to the dead letter queue associated to the source SQS queue"
Link to DLQ dev guide - https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-dead-letter-queues.html
msailes
reviewed
Aug 24, 2021
| * | ||
| * If there is no DLQ configured on source SQS queue and {@link SqsBatch#nonRetryableExceptions()} attribute is set, if | ||
| * nonRetryableExceptions occurs from {@link SqsMessageHandler}, such exceptions will still be treated as temporary | ||
| * exceptions and the message will be move back to source SQS queue for reprocessing. Same behaviour occurs if for some |
msailes
reviewed
Aug 24, 2021
| * | ||
| * If there is no DLQ configured on source SQS queue and {@link SqsBatch#nonRetryableExceptions()} attribute is set, if | ||
| * nonRetryableExceptions occurs from {@link SqsMessageHandler}, such exceptions will still be treated as temporary | ||
| * exceptions and the message will be move back to source SQS queue for reprocessing. Same behaviour occurs if for some |
There was a problem hiding this comment.
"The same behaviour will occur if for some reason the utility is unable to move the message to the DLQ."
"An example of this could be because the function is missing the correct permissions"
msailes
reviewed
Aug 24, 2021
| * If there is no DLQ configured on source SQS queue and {@link SqsBatch#nonRetryableExceptions()} attribute is set, if | ||
| * nonRetryableExceptions occurs from {@link SqsMessageHandler}, such exceptions will still be treated as temporary | ||
| * exceptions and the message will be move back to source SQS queue for reprocessing. Same behaviour occurs if for some | ||
| * reason utility is unable to move message to the DLQ. This can occur because of missing permissions. |
There was a problem hiding this comment.
What are the required permissions?
There was a problem hiding this comment.
I meant to document that as part of documentation update
pankajagrawal16
force-pushed
the
sqs-dlq-non-retryable
branch
2 times, most recently
from
7f8da88
to
2f68f48
Compare
|
Absolutely fantastic test coverage! |
pankajagrawal16
force-pushed
the
sqs-dlq-non-retryable
branch
2 times, most recently
from
ffb2171
to
f8220fe
Compare
msailes
previously approved these changes
Aug 25, 2021
| @@ -131,6 +131,57 @@ public static void overrideSqsClient(SqsClient client) { | |||
| return batchProcessor(event, false, handler); | |||
| } | |||
|
|
|||
| /** | |||
| * This utility method is used to processes each {@link SQSMessage} inside received {@link SQSEvent} | |||
| * This utility method is used to processes each {@link SQSMessage} inside received {@link SQSEvent} | ||
| * | ||
| * <p> | ||
| * Utility will take care of calling {@link SqsMessageHandler#process(SQSMessage)} method for each {@link SQSMessage} |
| * </p> | ||
| * | ||
| * <p> | ||
| * If any exception is thrown from {@link SqsMessageHandler#process(SQSMessage)} during processing of a messages, |
There was a problem hiding this comment.
"during processing of a message"
| * | ||
| * <p> | ||
| * If any exception is thrown from {@link SqsMessageHandler#process(SQSMessage)} during processing of a messages, | ||
| * Utility will take care of deleting all the successful messages from SQS. When one or more single message fails |
| * </p> | ||
| * | ||
| * <p> | ||
| * If you dont want to utility to throw {@link SQSBatchProcessingException} in case of failures but rather suppress |
| @@ -255,12 +484,12 @@ public static void overrideSqsClient(SqsClient client) { | |||
|
|
|||
| try { | |||
| if (null == handler.getDeclaringClass()) { | |||
| return handler.newInstance(); | |||
| return handler.getDeclaredConstructor().newInstance(); | |||
There was a problem hiding this comment.
What was the reason to change this?
There was a problem hiding this comment.
It was deprecated
added 4 commits
August 25, 2021 11:15
pankajagrawal16
force-pushed
the
sqs-dlq-non-retryable
branch
from
f8220fe
to
3b38a4d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: aws-powertools/powertools-lambda#29
Description of changes:
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.