feat: SQS Partial failure #100
Conversation
|
This pull request introduces 1 alert when merging 6924519 into 40972d2 - view on LGTM.com new alerts:
|
gmcrocetti
force-pushed
the
partial-sqs-batch
branch
from
6924519 to
3580f57
Compare
|
@cakepietoast could you help review when you get a chance :)? I'll finish mine tomorrow, and have bandwidth next week if you can't |
There was a problem hiding this comment.
Thanks for the awesome contribution so far @gmcrocetti! Looks like its on the right track to me. I added a few inline comments, happy to take another look when we have tests/docs for the functionality.
Thx for your fast review. I've left some replies :). |
|
Currently on vacation I will have a more in-depth look next week, but in general I am missing docs and tests. But I assume you concentrated on the implementation first? |
|
hey @gmcrocetti don't worry about the docs, any basic text for us to understand the UX would be helpful - If you allow us to make commits directly to you fork, we can help you craft the docs ;). Implementation and tests are the most important This is the first utility we launched yesterday ICYMI: https://awslabs.github.io/aws-lambda-powertools-python/utilities/parameters/ |
Hi @heitorlessa , sorry to take so long, it was quite a busy week. I'm hoping to finish tests and docs this week (tomorrow in fact), is it possible to follow this schedule :) ? I'll push my progress ASAP so you can follow. BTW, this link will help me a ton. |
|
Absolutely - There’s no rush ;)
I’m not sure if it was discussed in the review yet, but I don’t think
DynamoDB and Kinesis can have their records deleted in this manner, so this
would only work for SQS.
Super looking forward to reviewing it, this will be awesome
…On Mon, 24 Aug 2020 at 17:10, Guilherme Martins Crocetti < ***@***.***> wrote:
hey @gmcrocetti <https://github.com/gmcrocetti> don't worry about the
docs, any basic text for us to understand the UX would be helpful - If you
allow us to make commits directly to you fork, we can help you craft the
docs ;). Implementation and tests are the most important
This is the first utility we launched yesterday ICYMI:
https://awslabs.github.io/aws-lambda-powertools-python/utilities/parameters/
Hi @heitorlessa <https://github.com/heitorlessa> , sorry to take so long,
it was quite a busy week. I'm hoping to finish tests and docs this week
(tomorrow in fact), is it possible to follow this schedule :) ?
I'll push my progress ASAP so you can follow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#100 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBD7S6ZIE2ON7HM5ILDSCJ7FLANCNFSM4P5XTMVQ>
.
|
|
Yeap, you're totally right ! It wasn't touched by no means but I think we must at least discuss this point. How can we proceed if we'll not be able to delete these failed records for both cases ? Do you have any idea ? Even we don't tackle this right now, it's a good point because we may avoid design flaws. |
|
My suggestion would be to focus on SQS only, thus simplifying the
implementation - I'm happy to write a recommendation for customers using
Kinesis and DynamoDB streams under a Tips section in your docs. The new
async controls for stream processing in Lambda allow them to prevent poison
pills, and also be more flexible on not allowing a retry, or simply
bisecting a failed record.
I'll also write that later in the year when we refresh Serverless best
practices given the new controls that are now in place. SQS is the
exception as you can't control that, so you need something like your
implementation if you don't want to implement idempotency downstream and
onwards (harder).
Does that make sense?
…On Mon, 24 Aug 2020 at 20:16, Guilherme Martins Crocetti < ***@***.***> wrote:
Yeap, you're totally right ! It wasn't touched by no means but I think we
must at least discuss this point.
How can we proceed if we'll not be able to delete these failed records for
both cases ? Do you have any idea ?
Even we don't tackle this right now, it's a good point because we may
avoid design flaws.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#100 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBGAL2TDESGZFHSXA2TSCKU6BANCNFSM4P5XTMVQ>
.
|
|
Yes, makes a lot of sense. In fact, that's why I was working solely with the sqs processor. These async controllers are pretty awesome, and even smth "simple" like "destination on failure" helps a lot. A "tips" section would be great ! |
gmcrocetti
force-pushed
the
partial-sqs-batch
branch
from
3580f57 to
b92b15e
Compare
Codecov Report
@@ Coverage Diff @@
## develop #100 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 28 +4
Lines 706 784 +78
Branches 67 72 +5
=========================================
+ Hits 706 784 +78
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@gmcrocetti nice implementation I left some questions/remarks hopefully you will find them helpful.
|
@heitorlessa , finally did it ! Phewwww !! Waiting reviews. Thx |
…ace always expecting a BatchProcessor
gmcrocetti
force-pushed
the
partial-sqs-batch
branch
from
68905a5 to
ded3d75
Compare
There was a problem hiding this comment.
@gmcrocetti nice work! Coming along nicely did spot a naming improvement and I believe a copy paste error
There was a problem hiding this comment.
This is a huge improvement from the initial version - Thank you for all effort you've put in already
Made some comments to help clarify a few areas - I'm happy to contribute some docs later if you don't have the bandwidth, but the implementation itself looks solid.
I might make a comment on middlewares.py to reorganize that into a processor.py/something else I can't think of yet, but we can contribute to that later too, don't worry.
|
Thanks a lot @gmcrocetti for the changes - Much cleaner, and we appreciate very much all effort you've put on to this! We're merging as-is. @cakepietoast will create another PR to clean up the docs for consistency, and will cut a 1.5.0 release on Friday that will contain this and other enhancements we've made ;) |
|
Happy to see it merged ! Thx for all support ! |
#92 : SQS Partial batch
Description of changes:
Hi everyone ! Here's a very initial design proposal to work with batches. This draft intends to work as our communication channel to hear your toughts. Before putting more effort in everything that's missing (docs, tests, typing, etc) I look forward hearing opinions from issue's participants: @heitorlessa , @nmoutschen , @Nr18 .
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.