fix(batch): delete >10 messages in legacy sqs processor #818
Conversation
…e than 10 messages were processed that would need deleted.
pull-request-size
bot
added
the
size/XS
whardier
changed the title
|
Working on updating the tests now.. overlooked it. |
pull-request-size
bot
removed
the
size/XS
pull-request-size
bot
added
the
size/S
Codecov Report
@@ Coverage Diff @@
## develop #818 +/- ##
===========================================
- Coverage 99.96% 99.90% -0.06%
===========================================
Files 119 119
Lines 5347 5364 +17
Branches 610 612 +2
===========================================
+ Hits 5345 5359 +14
- Misses 0 3 +3
Partials 2 2
Continue to review full report at Codecov.
|
|
Hey @whardier could you create an issue first detailing the bug please? For example, what configuration are you using so that your function receives more than 10 messages per invocation where this is necessary? e.g. large batch size, longer batch window? If a single function receives a few thousand records with 50% partial failure, this has the side effect of increasing function duration and potentially timing out if one is not careful. An issue helps us think about potential side effects like these, compromises, what information we need to provide in the docs to help other customers unfamiliar with this scenario, etc. Thanks a lot! |
heitorlessa
changed the title
|
@mploski before you tackle the redis one, could you help revamp this PR so we can merge it? Ask. Move to concurrent futures and pass the client to make it thread safe as explained in the review Background. This is the legacy batch we keep around that we're only deleting 10 messages instead of all failed messages in a batch (could be hundreds). |
heitorlessa
added
help wanted
Sure @heitorlessa! will look at this at the end of the next week. |
|
@whardier @heitorlessa I will make changes on top of this PR's branch today. |
|
@whardier Could you fetch latest changes from upstream into your repo clone? You're lagging behind a bit :-) |
pull-request-size
bot
added
size/M
mploski
force-pushed
the
fix/batch-sqs-handler-delete-message-batch-needs-batched
branch
from
6a8fc66 to
9333e14
Compare
|
@whardier @heitorlessa Applied suggestions, please review. |
|
This looks clean. I can't test it right away however. I like it. |
| @@ -120,23 +123,39 @@ def _prepare(self): | |||
| self.success_messages.clear() | |||
| self.fail_messages.clear() | |||
|
|
|||
| def _clean(self): | |||
| def _clean(self) -> Optional[List]: | |||
heitorlessa
changed the title
No related issue
Description of changes:
SQS/Batch - SImple addition to process chunks of records that need removed in 10's. Counterintuitive and migrating this behaviour to the batch processor scope itself would be too disruptive.
Checklist
Breaking change checklist
No breaking changes. Introduces no new exception handling and would fix batch handling of more than 10 records.
No RFC:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.