Include failure details in SendBatchOperationFailedException message#1608
Include failure details in SendBatchOperationFailedException message#1608krechanski wants to merge 3 commits into
Conversation
…exception-failure-mesage' into fix-send-batch-operation-failed-exception-failure-mesage
krechanski
requested review from
MatejNedic,
maciejwalkowiak and
tomazfernandes
as code owners
tomazfernandes
left a comment
tomazfernandes
left a comment
There was a problem hiding this comment.
@krechanski, thanks for the PR, left a couple of comments.
| private <T> CompletableFuture<SendResult.Batch<T>> handleFailedSendBatch(String endpoint, | ||
| SendResult.Batch<T> result) { | ||
| return CompletableFuture.failedFuture(new SendBatchOperationFailedException("", endpoint, result)); | ||
| SendResult.Batch<T> result) { |
There was a problem hiding this comment.
Please do not use spaces to align, you can run mvn spotless:apply to fix this.
| result.failed().size() + result.successful().size(), endpoint, | ||
| MessageHeaderUtils.getId(result.failed().stream().map(SendResult.Failed::message).toList()), | ||
| result.failed().stream().map(SendResult.Failed::errorMessage) | ||
| .collect(Collectors.joining("; "))), endpoint, result)); |
There was a problem hiding this comment.
My concern here would be that we already use ; to join the ids in MessageHeaderUtils.getId().
How about we pair the ids with the corresponding error message instead?
Also, let's guard against a possibly null message.
|
@MatejNedic, it seems the Validate Docs action is failing due to an issue in Would you mind fixing it so we have a green docs build? |
|
@tomazfernandes fix is in main. Rebase against main and we are good! |
|
Thanks @MatejNedic for the fix. @krechanski, if you can rebase onto the updated main so we pull it in, thanks. |
tomazfernandes
added
the
status: waiting-for-feedback
3 participants
📢 Type of change
📜 Description
SendBatchOperationFailedExceptionwas constructed with an empty message string, making it impossible to diagnose batch send failures from the exception message alone. The message now includes the number of failed vs total messages, the target endpoint, the failed message IDs, and the error messages from AWS.💡 Motivation and Context
When a batch send partially fails, the thrown exception carried no useful information in its message. Callers relying on logging or error reporting had to inspect the exception object programmatically to understand what went wrong.
💚 How did you test it?
Added unit test
shouldIncludeFailureDetailsInBatchExceptionMessageinSqsTemplateTeststhat simulates a partial batch failure and asserts the exception message contains the failed/total count, endpoint name, failed message ID, and error message.📝 Checklist
🔮 Next steps
Run the full test suite to confirm no regressions.