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

feat: New batch processing option: 'ThrowOnFullBatchFailure' #646

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

lachriz-aws
Copy link
Contributor

Note

I still have to update the docs, which is why this PR is created as draft.

--

Added new batch processing option: ThrowOnFullBatchFailure to control if a BatchProcessingException should be raised on full batch failure or not.

Please provide the issue number

Issue number: #608

Summary

Changes

Please provide a summary of what's being changed

This PR introduces a new batch processing option: ThrowOnFullBatchFailure to control if a BatchProcessingException should be raised on full batch failure or not.

The processing option can be set via:

  • A new ThrowOnFullBatchFailure field on the BatchProcessor attribute (applied in non-utility mode).
  • A new environment variable POWERTOOLS_BATCH_THROW_ON_FULL_BATCH_FAILURE (applied in non-utility mode).
  • Directly on the AWS.Lambda.Powertools.BatchProcessing.ProcessingOptions in utility mode.

To maintain current behavior, the new setting defaults to true, but as pointed out on a similar feature request in the Powertools for AWS Lambda (TypeScript) repo, here: aws-powertools/powertools-lambda-typescript#2122 we might want to change that for the next major version release.

To maintain feature parity, the new setting has been named similar to how it has been implemented in Powertools for AWS Lambda (TypeScript), see: https://github.com/arnabrahman/aws-lambda-powertools-typescript/blob/main/packages/batch/src/types.ts#L32

User experience

Please share what the user experience looks like before and after this change

Imagine a scenario where processing of an entire batch has failed: if ThrowOnFullBatchFailure is set to false then the Batch Processor will not throw an exception after batch processing has completed. Instead, it will simply return the ids of the items that failed processing (all batch items in this case) and exit gracefully.

In the scenario that a Lambda function is configured with ErrorHandlingPolicy = StopOnFirstBatchItemFailure and the first batch item fails processing, then the entire batch is marked as failed (as per the docs: https://docs.powertools.aws.dev/lambda/dotnet/utilities/batch-processing/#error-handling-policy). In this case, if ThrowOnFullBatchFailure is set to false, then the same behavior as outlined above will apply (in other words, the new ThrowOnFullBatchFailure option is compatible with the different error handling policies we have today).

Checklist

Please leave checklist items unchecked if they do not apply to your change.

Is this a breaking change? No

RFC issue number:

Checklist:

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

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

…ol if a BatchProcessingException should be raised on full batch failure or not.
@boring-cyborg boring-cyborg bot added area/common Core Powertools utility tests labels Sep 19, 2024
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 19, 2024
@lachriz-aws lachriz-aws changed the title Added new batch processing option: 'ThrowOnFullBatchFailure' feat: New batch processing option: 'ThrowOnFullBatchFailure' Sep 19, 2024
@github-actions github-actions bot added the feature New features or minor changes label Sep 20, 2024
@hjgraca
Copy link
Contributor

hjgraca commented Sep 20, 2024

@lachriz-aws thank you for opening the pull request. There are two small issues raised by SonarCloud. https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=646&id=aws-powertools_powertools-lambda-dotnet&open=AZILxhrgZTYLizOkweVb. Can you address them please? thank you

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.51%. Comparing base (5a483b7) to head (e1d88c2).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #646      +/-   ##
===========================================
- Coverage    72.71%   72.51%   -0.20%     
===========================================
  Files          190      190              
  Lines         7886     7893       +7     
  Branches       850      850              
===========================================
- Hits          5734     5724      -10     
- Misses        1862     1875      +13     
- Partials       290      294       +4     
Flag Coverage Δ
unittests 72.51% <100.00%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Sep 20, 2024

@lachriz-aws
Copy link
Contributor Author

@lachriz-aws thank you for opening the pull request. There are two small issues raised by SonarCloud. https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=646&id=aws-powertools_powertools-lambda-dotnet&open=AZILxhrgZTYLizOkweVb. Can you address them please? thank you

Yes, absolutely. Now done.

When time allows, please let me know what you think of the proposed solution and I will be happy to get going updating the docs as well. Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/common Core Powertools utility feature New features or minor changes size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants