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

Add configurable visibility timeout default when using partial batch failures #151

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

brendonparker
Copy link
Contributor

Issue #150

Description of changes:

Adds a configuration option to LambdaMessagingOptions for a VisibilityTimeout value when there are partial batch failures. In that scenario, the failed messages will have their message visibility set to this default value.

Prior to this change, the message visibility would not be set/changed. Which, if you were using a FIFO queue, would mean that other messages in that message group would be blocked until the original message visibility timeout occurred, which could be a while.

After this change, the message visibility will be changed on failure.

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

@brendonparker
Copy link
Contributor Author

I made this only applicable when UseBatchResponse is true. I think it could be changed to work regardless of that, if so desired.

Copy link
Contributor

@normj normj left a comment

Choose a reason for hiding this comment

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

I have a minor style nit but the change looks good. Thanks for the PR!

/// </summary>
private async Task ResetVisibilityTimeoutForFailures()
{
if (_configuration.SQSEvent == null) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Style wise can you either use brackets or put the return on a separate indented line?

@ashovlin ashovlin removed their request for review August 2, 2024 18:30
@philasmar
Copy link
Contributor

@brendonparker I have ran the integration tests and they have passed for this PR. Once you address Norm's comments, I can squash your commits and release the changes.

@brendonparker
Copy link
Contributor Author

@philasmar done

@philasmar philasmar merged commit c42c72c into awslabs:dev Aug 2, 2024
5 of 6 checks passed
@philasmar
Copy link
Contributor

This change was released in AWS.Messaging.Lambda (0.10.0).

Thank you for your contribution @brendonparker!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants