Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: batch processing util #155

Merged
merged 9 commits into from
Sep 3, 2020
Merged

Conversation

to-mc
Copy link
Contributor

@to-mc to-mc commented Sep 2, 2020

Issue #, if available:

Description of changes:

Simplified the documentation, primarily by focusing more on the SQS specific implementation. Added a sqs_batch_processor interface to simplify the common use case. Fixed an issue where no exception was being raised for failed messages, which would have caused failed messages to be deleted from the queue.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tom McCarthy added 2 commits September 2, 2020 17:23
more SQS specific focus
Update for sqs_batch_processor interface
@to-mc to-mc marked this pull request as draft September 2, 2020 15:45
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #155   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines          806       827   +21     
  Branches        76        77    +1     
=========================================
+ Hits           806       827   +21     
Impacted Files Coverage Δ
aws_lambda_powertools/utilities/batch/__init__.py 100.00% <100.00%> (ø)
aws_lambda_powertools/utilities/batch/base.py 100.00% <100.00%> (ø)
...ws_lambda_powertools/utilities/batch/exceptions.py 100.00% <100.00%> (ø)
aws_lambda_powertools/utilities/batch/sqs.py 100.00% <100.00%> (ø)

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 9c09099...285054d. Read the comment docs.

@to-mc to-mc marked this pull request as ready for review September 3, 2020 07:22
@to-mc to-mc changed the title docs: batch processing util fix: batch processing util Sep 3, 2020
records = event["Records"]

with processor(records, record_handler):
processor.process()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a logger.debug here to provide a message that you're processing another record. We don't need to log the record for security reasons, just a message suffices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outside of the loop for processing single records, but I've added debug logging elsewhere which should suffice.


**Background**

When using SQS as a Lambda event source mapping, functions can be triggered with a batch of messages from SQS. If the Lambda function fails when processing the batch,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some line breaks to make it easier to read. The docs theme kinda forces you to do that or else it cramps the lines


### PartialSQSProcessor
**With a decorator:**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a sub-heading #### as opposed to bold text - It allows customers and us to easily reference it in conversations


SQS integration with Lambda is one of the most well established ones and pretty useful when building asynchronous applications. One common approach to maximize the performance of this integration is to enable the batch processing feature, resulting in higher throughput with less invocations.
Using the `sqs_batch_processor` decorator with your lambda handler function, you provide a `record_handler` which is responsible for processing individual messages. It should raise an exception if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add line breaks here to make it easier to read in our docs theme


A *naive* approach to solving this problem is to delete successful records from the queue before redriving's phase. The `PartialSQSProcessor` class offers this solution both as context manager and middleware, removing all successful messages from the queue case one or more failures occurred during lambda's execution. Two examples are provided below, displaying the behavior of this class.
**With a context manager:**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a sub-heading #### as opposed to bold text - It allows customers and us to easily reference it in conversations

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions to break middlewares.py functionalities into base.py and sqs.py to be closer to their respective implementations, debug logging, and docs to make it easier to read.

I'll work on the docs as it's a quick one for me

@heitorlessa heitorlessa added area/utilities bug Something isn't working labels Sep 3, 2020
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Docs look super clean now too, and great to catch that failed record bug before it's released :)

@heitorlessa heitorlessa merged commit 3be8d03 into develop Sep 3, 2020
@heitorlessa heitorlessa deleted the docs/batch_processing_util branch September 3, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants