Add support for SqsTemplate to send more than 10 messages at once - #1659
Add support for SqsTemplate to send more than 10 messages at once#1659joseiedo wants to merge 21 commits into
Conversation
Add more tests simplifying tests stop sequential send when a failure occurs
tomazfernandes
left a comment
There was a problem hiding this comment.
Thanks for the PR @joseiedo. I see you're still pushing commits, so I'll leave just one comment about the partitioning strategy. Let me know your thoughts.
| int totalSize = messagesToUse.size(); | ||
| return IntStream.rangeClosed(0, (totalSize - 1) / pageSize) | ||
| .mapToObj(index -> messagesToUse.subList(index * pageSize, Math.min((index + 1) * pageSize, totalSize))) | ||
| .mapToObj(index -> (Collection<T>) messagesToUse.subList(index * pageSize, Math.min((index + 1) * pageSize, totalSize))) |
There was a problem hiding this comment.
I'm not sure why suddenly this was happening, but the pipeline couldn't compile successfully if I didn't used this cast.
https://github.com/awspring/spring-cloud-aws/actions/runs/30174435089/job/89720833992
|
I added a binpack algorithm here, let me know your thoughts about it! |
tomazfernandes
left a comment
There was a problem hiding this comment.
Thanks for the update @joseiedo, the partitioning logic looks great.
One thing left to adjust is that currently if one sending operation in a batch fails, we'll throw the error and some messages will have been sent and others won't.
For the single batch (<= 10 messages) this works because it's all-or-nothing anyway, but for multiple batches we need a way of letting the user know what was sent and what was not.
My suggestion would be to convert exceptions to Failed entries at the partition boundary, so every future entering combineBatchFutures and the sequential chains always completes normally. Something along the lines of:
private <T> CompletableFuture<SendResult.Batch<T>> sendPartitionedBatch(String endpointName, Collection<Message> partition, Map<String, org.springframework.messaging.Message<T>> originalMessagesById) {
return sendSingleBatch(endpointName, partition, originalMessagesById).exceptionally(t -> createFailedBatchResult(partition, t, endpointName, originalMessagesById));
}Where createFailedBatchResult would mirror createSkippedResult, with the unwrapped cause (strip CompletionException) in errorMessage and additionalInformation. With the default THROW strategy the user still gets SendBatchOperationFailedException, just carrying the full result now.
What do you think?
|
Hi @tomazfernandes, I did the adjustment. For the |
tomazfernandes
left a comment
There was a problem hiding this comment.
Thanks for the updates @joseiedo, looks good.
I've left a few minor final comments.
Please also update the docs for this new feature (since 4.2.0).
If possible, add a test with the automatic group id scenario: more than 10 messages without group id and asserting that it was batched correctly, e.g. 25 messages should result in 3 requests.
… failed batch results
|
Hi @tomazfernandes, changes done! For the documentation I replaced only the part where it talks about the limitation, let me know if you think it's necessary to have a section with a more detailed explanation or if it's ok. |
|
Just a sec. missed one adjustment, the one adding the cause itself in the additionalInformation instead of the exception classname. |
|
Now it's done. Please, share your thoughts! |
📢 Type of change
📜 Description
AWS SDK doesn't have support to send a batch of messages of more than 10 items. This PR add an alternative where the received batch is partitioned in smaller batches of 10. For FIFO queues the batches are sent sequentially (if one fails, the others aren't sent), while for non-FIFO they are parallelized.
💡 Motivation and Context
solves #1042
💚 How did you test it?
I added integration test and unit tests with a sample.
📝 Checklist
🔮 Next steps
Check if there's concerns with this approach and update documentation explaining how this works in order to make it clear for users.