-
-
Notifications
You must be signed in to change notification settings - Fork 294
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
Docs: Add new SQS sample (manual container instantiation) #774
Conversation
Hi @tomazfernandes, how are you? Can you see if it is a minimum code to use acknowledge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mcruzdev, thanks for the PR!
Left a few comments - please let me know if you have any questions or suggestions.
|
||
public static final String NEW_USER_QUEUE = "new-user-queue"; | ||
|
||
// Change to 'true' if you want to not ack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we need this Boolean - we can just always ack in the listener.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to the user see the negative side ( non-acknowledge ) and to see the messages in flight. But it can be confused.
@Bean | ||
public ApplicationRunner sendMessageToQueue(SqsTemplate sqsTemplate) { | ||
LOGGER.info("Sending message"); | ||
return args -> sqsTemplate.sendAsync(to -> to.queue(NEW_USER_QUEUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the sync send method here - this way if an error occurs it will throw instead of returning a failed future.
|
||
@Bean | ||
SqsMessageListenerContainer<User> sqsMessageListenerContainer(SqsAsyncClient sqsAsyncClient) { | ||
SqsMessageListenerContainer<User> container = new SqsMessageListenerContainer<>(sqsAsyncClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is a perfectly good use case for manual ack, I think it illustrates more instantiating a container manually - which is a more advanced feature - than the manual ack itself.
So I'm thinking we keep this one as a Manual Container Instantiation sample, and maybe create another one configuring the manual ack in the MessageListenerContainerFactory and using the regular @SqsListener
?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I already created both here and I can add it in the next PR.
@Override | ||
public void onSuccess(Collection<Message<User>> messages) { | ||
LOGGER.info("Ack with success"); | ||
AcknowledgementResultCallback.super.onSuccess(messages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to call super here - this is just a callback for user logic.
Hi @tomazfernandes I moved this pull request to this one #788, changing the branch name and I created a new pull request for manual ack sample #787 |
📢 Type of change
📜 Description
This pull request adds a new sample for SQS, in other words this add a sample showing how can use manual ack with SQS.
It is a new pull request for issue #670
💡 Motivation and Context
The motivation is to make a better documentation with code samples.
💚 How did you test it?
📝 Checklist
🔮 Next steps