Prevent a batch of messages going over the threshold #37
Conversation
| @@ -1 +1,3 @@ | |||
| target/ | |||
| .idea/ | |||
| *.iml | |||
There was a problem hiding this comment.
Useful extra .ignores my end.
| @@ -61,7 +61,7 @@ | |||
| private static final int MORE_THAN_SQS_SIZE_LIMIT = SQS_SIZE_LIMIT + 1; | |||
|
|
|||
| // should be > 1 and << SQS_SIZE_LIMIT | |||
| private static final int ARBITRATY_SMALLER_THRESSHOLD = 500; | |||
| private static final int ARBITRARY_SMALLER_THRESHOLD = 500; | |||
| @@ -158,9 +158,43 @@ public void testReceiveMessageMultipleTimesDoesNotAdditionallyAlterReceiveMessag | |||
| Assert.assertEquals(expectedRequest, messageRequest); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testWhenSmallMessageBatchIsSentThenNoMessagesStoredInS3() { | |||
There was a problem hiding this comment.
Missing test that proves that a perfectly small batch is not moved to s3
| @@ -172,7 +206,7 @@ public void testWhenMessageBatchIsSentThenOnlyMessagesLargerThanThresholdAreStor | |||
| 700_000, | |||
| 800_000, | |||
| 900_000, | |||
| 200_000, | |||
| 20_000, | |||
There was a problem hiding this comment.
Needed to make this smaller so that the messages not sent to s3 did not themselves exceed the batch size in this instance.
| 26_214, | ||
| 26_214, | ||
| 26_214, | ||
| 26_219 |
There was a problem hiding this comment.
Small messages which just exceed the batch max
| } | ||
|
|
||
| private boolean isLarge(SendMessageBatchRequestEntry batchEntry) { | ||
| private boolean isLarge(long size) { | ||
| return (size > clientConfiguration.getMessageSizeThreshold()); |
There was a problem hiding this comment.
Reused isLarge moved into its own function.
| return (size > clientConfiguration.getMessageSizeThreshold()); | ||
| } | ||
|
|
||
| private long sizeOf(SendMessageBatchRequestEntry batchEntry) { |
There was a problem hiding this comment.
sizeOf means we can work out the sums of batch members.
| @@ -775,15 +773,41 @@ public SendMessageBatchResult sendMessageBatch(SendMessageBatchRequest sendMessa | |||
|
|
|||
| List<SendMessageBatchRequestEntry> batchEntries = sendMessageBatchRequest.getEntries(); | |||
|
|
|||
| List<Integer> smallMessages = new LinkedList<>(); | |||
There was a problem hiding this comment.
Now this is a two pass process. Move the obviously big ones to s3, catching the indices of those we didn't move.
|
|
||
| if (isLarge(totalSize) && !smallMessages.isEmpty()) { | ||
| moveRemainingMessagesToS3(batchEntries, smallMessages); | ||
| } |
There was a problem hiding this comment.
If necessary, then we move the remainder
|
Hey, can this one get any attention? It's a real issue with the client that needs to be addressed. |
|
Take a fork of my version and use that. It's what we did with my last client. AWS tends not to fix these releases. |
|
@ashleyfrieze i got my PR merged here few month ago, so there is some hope this one will also be reviewed if we will create some noise here :) |
|
@ashleyfrieze It looks like this had been approved previously? but has since conflicts now. Is this something that could be re-visited to push? Add another to the list of projects it has bitten trying to use sendBatch |
|
@ymartin we solved this problem - the lack of merge - by building my fork and publishing it to a local repo. That was back in 2019 or so. I haven’t the time to pick this up myself. However, if you want to get a commit into shape on your own fork and then ask me to make my code look like that commit behind this PR, I’m happy to do so. It’ll take a little bit of git magic, but it is easy to do. |
|
@ymartin I thought I'd have ago at resolving the conflicts. However, as I feared, the problem is made more complex by the fact that the latest version has switched to a newer AWS SDK. So, which SDK do we want to use for this fix? Probably SDK v2? |
Issue #24
Description of changes:
I noticed that the sum of the message sizes in the batch could still go over the threshold, so I've added support, and tests, for this to be prevented. I'll comment on the PR in order to explain the changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.