Skip to content

ImmediateRetryAsyncErrorHandler changes msg visibility timeout to zero. (#1314) #1370

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rafaelcgpava
Copy link

@rafaelcgpava rafaelcgpava commented Apr 11, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

New ImmediateRetryAsyncErrorHandler to change message visibility timeout to zero when an error occurs

💡 Motivation and Context

As mentioned in (#1313), it was not possible to immediatly report back an error to AWS SQS. This enhancement allows this feature to immediatly make the message available for retry by changing it's visibility timeout to zero.

See #1314

💚 How did you test it?

Unit test, Integration test, manual tests with a sample application

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

  • Update sample application
  • Develop ImmediateRetryErrorHandler

@github-actions github-actions bot added the component: sqs SQS integration related issue label Apr 11, 2025
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Hey Bruno and Rafael, thanks for the PR!

I left a few comments, please let me know if they make sense to you or if you have any questions.

Thanks!

@rafaelcgpava
Copy link
Author

Hey Bruno and Rafael, thanks for the PR!

I left a few comments, please let me know if they make sense to you or if you have any questions.

Thanks!

Hey there Tomaz! Thank you for reviewing our PR.

Your suggestions make a lot of sense and we have just commited the changes.

Please let us know if there is anything else we could change/improve.

@rafaelcgpava rafaelcgpava changed the title AsyncDefaultErrorHandler changes msg visibility timeout to zero. (#1314) ImmediateRetryAsyncErrorHandler changes msg visibility timeout to zero. (#1314) Apr 11, 2025
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Hey @rafaelcgpava, thanks for the adjustments!

This looks good, I've just left a couple more details for your consideration.

I'll also ask you to add this feature to the docs (sqs.adoc), including how to set it up (just declare it as a bean for auto-configuration, or add to a Factory / Container directly).

Please let me know if you have any questions / suggestions.

@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Apr 18, 2025
@brun0-4ugusto
Copy link

Hey @tomazfernandes, thank you for the review!

We've just pushed the requested changes. Please let us know if there's anything else we should revise or improve. 🎉 🚀

Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

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

Almost there @rafaelcgpava and @brun0-4ugusto!

@rafaelcgpava
Copy link
Author

rafaelcgpava commented Apr 18, 2025

@tomazfernandes once again, thank you very much for your feedback and the suggestions you have been making! It has been a great learning experience!

I´ve just commited the changes we've made from your suggestions =)

I had a question about the note we´ve left saying that there was only the AsyncErrorHandler implementation for ImmediateRetry, however I checked that we adapt ErrorHandlers to AsyncErrorHandlers using BlockingErrorHandlerAdapter, and it makes a lot of sense to remove that note.

@brun0-4ugusto
Copy link

Hi @tomazfernandes! I hope this message finds you well. I just wanted to kindly check if there are any changes needed on the PR — I’m available to assist with any updates you might require.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sqs SQS integration related issue type: documentation Documentation or Samples related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants