Skip to content

feat(batch): add option to continue processing other group IDs on failure in SqsFifoPartialProcessor #2590

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

Merged

Conversation

arnabrahman
Copy link
Contributor

@arnabrahman arnabrahman commented May 29, 2024

Summary

When using SQS FIFO, by default, we will stop processing at the first failure and mark unprocessed messages as failed to preserve ordering. However, this behavior may not be optimal for customers who wish to proceed with processing messages from a different group ID.

Here an option has been given to the customer to skip the default behavior.

Changes

  • Introduced a skipGroupOnError option that can be passed to the processPartialResponseSync function.
  • Made a generic type so that skipGroupOnError is accepted only when the processor parameter is of type SqsFifoPartialProcessor. Example:
         const processor = new SqsFifoPartialProcessor();
         const result = processPartialResponseSync(
            event,
            sqsRecordHandler,
            processor,
            {
              context,
              skipGroupOnError: true,
            }
          );
  • If skipGroupOnError is not true, then short-circuit the process on first failure same as before.
  • If skipGroupOnError is true, then continue processing on the failure. Only if the current group has a previous failure then skip processing the current record and log SqsFifoMessageGroupShortCircuitError. Otherwise, process the record.
  • Write unit tests for skipGroupOnError flag
  • Updated the documentation, added examples and added sequence diagrams. I followed the Python powertools documentation for reference.

Issue number: #2561


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.

@boring-cyborg boring-cyborg bot added batch This item relates to the Batch Processing Utility documentation Improvements or additions to documentation tests PRs that add or change tests labels May 29, 2024
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label May 29, 2024
@arnabrahman
Copy link
Contributor Author

Ok, there are some merge conflicts. Will fix it and then make this ready.

@arnabrahman arnabrahman marked this pull request as ready for review May 31, 2024 12:30
@arnabrahman arnabrahman requested a review from a team May 31, 2024 12:30
@arnabrahman arnabrahman requested a review from a team as a code owner May 31, 2024 12:30
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jun 3, 2024
@dreamorosi
Copy link
Contributor

Hi @arnabrahman - thank you for your patience on this.

I've just got back from PTO and will review this in the next 24hrs tops.

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thank you so much for the work on this PR, the change is super clean and I really appreciate that you went the extra mile and also updated the docs!

I have left only a few minor comments, after which I am looking forward to merge the PR and include the feature in this week's release :)

@arnabrahman arnabrahman marked this pull request as draft June 4, 2024 03:44
@dreamorosi dreamorosi marked this pull request as ready for review June 4, 2024 12:48
@dreamorosi dreamorosi self-requested a review June 4, 2024 12:53
…hman/aws-lambda-powertools-typescript into 2561-improve-sqs-fifo-processing
Copy link

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
29.8% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thank you for the thorough work on this PR, I really appreciate the attention to details and openness to work on iterations during the review.

Merging this now 🎉

PS: I've labelled two more issues (list) related to this utility as help wanted just in case you're interested. They're both in similar complexity and along the lines of this one.

@dreamorosi dreamorosi merged commit a615c24 into aws-powertools:main Jun 4, 2024
11 checks passed
@arnabrahman arnabrahman deleted the 2561-improve-sqs-fifo-processing branch June 4, 2024 12:59
@arnabrahman
Copy link
Contributor Author

arnabrahman commented Jun 4, 2024

Nice, i will take a look after finishing 1774 , @dreamorosi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
batch This item relates to the Batch Processing Utility documentation Improvements or additions to documentation feature PRs that introduce new features or minor changes size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: allow SqsFifoPartialProcessor to continue processing other group IDs upon failure
2 participants