feat: SQS Partial failure

[gmcrocetti]

#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

• Meet tenets criteria
• [-] Update tests
• [-] Update docs
• PR title follows conventional commit semantics

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.

[lgtm-com]

This pull request introduces 1 alert when merging 6924519 into 40972d2 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[heitorlessa]

@cakepietoast could you help review when you get a chance :)?

I'll finish mine tomorrow, and have bandwidth next week if you can't

[to-mc]

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.

[gmcrocetti]

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 :).

[Nr18]

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?

[heitorlessa]

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/

[gmcrocetti]

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.

[heitorlessa]

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> .

[gmcrocetti]

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.

[heitorlessa]

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> .

[gmcrocetti]

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 !
Thanks

[codecov-commenter]

Codecov Report

Merging #100 into develop will not change coverage.
The diff coverage is 100.00%.

@@            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     

Impacted Files	Coverage Δ
..._lambda_powertools/utilities/parameters/secrets.py	100.00% <ø> (ø)
aws_lambda_powertools/utilities/batch/__init__.py	100.00% <100.00%> (ø)
aws_lambda_powertools/utilities/batch/base.py	100.00% <100.00%> (ø)
...s_lambda_powertools/utilities/batch/middlewares.py	100.00% <100.00%> (ø)
aws_lambda_powertools/utilities/batch/sqs.py	100.00% <100.00%> (ø)
aws_lambda_powertools/logging/logger.py	100.00% <0.00%> (ø)
aws_lambda_powertools/logging/formatter.py	100.00% <0.00%> (ø)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8da0cce...9caf3d1. Read the comment docs.

[Nr18]

@gmcrocetti nice implementation I left some questions/remarks hopefully you will find them helpful.

[gmcrocetti]

@heitorlessa , finally did it ! Phewwww !! Waiting reviews. Thx

[Nr18]

@gmcrocetti nice work! Coming along nicely did spot a naming improvement and I believe a copy paste error

[heitorlessa]

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.

[Nr18]

LGTM

[heitorlessa]

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 ;)

[gmcrocetti]

Happy to see it merged ! Thx for all support !