-
-
Notifications
You must be signed in to change notification settings - Fork 297
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
Ability to set SmartLifecycle.phase to SqsMessageListenerContainer/DefaultListenerContainerRegistry #821
Ability to set SmartLifecycle.phase to SqsMessageListenerContainer/DefaultListenerContainerRegistry #821
Conversation
…SmartLifecycle interface
…SmartLifecycle interface
...d-aws-sqs/src/main/java/io/awspring/cloud/sqs/listener/AbstractMessageListenerContainer.java
Show resolved
Hide resolved
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.
Looks great @estigma88 , thanks! Just one minor comment.
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.
Thanks a lot for the PR @estigma88!
Looking forward to more, there are quite a few open SQS issues, if you see any that you'd like to contribute just let us know.
Perhaps we should update reference docs with an explanation? |
Good call @maciejwalkowiak. @estigma88, would you mind adding a few lines to the doc regarding phase? Let me know if you need any help with that. Thanks! |
Sure @maciejwalkowiak @tomazfernandes , I will add some docs. Btw, I was checking a little deep over the sqs framework, and I found that |
…SmartLifecycle interface
@estigma88 sorry, I’m in the middle of lots of things here and couldn’t reply before. It makes sense to update the DefaultListenerContainerRegistry, good catch. Please add it to the PR if you can. Thanks. |
@tomazfernandes don't worry. I updated the docs, let me know if that makes sense, also, did a little refactor to reuse the |
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.
@estigma88 , just two comments about the documentation changes, please let me know if you have any questions.
Thanks.
docs/src/main/asciidoc/sqs.adoc
Outdated
@@ -927,6 +927,8 @@ The `MessageListenerContainer` interface extends `SmartLifecycle`, which provide | |||
Containers created from `@SqsListener` annotations are registered in a `MessageListenerContainerRegistry` bean that is registered by the framework. | |||
The containers themselves are not Spring-managed beans, and the registry is responsible for managing these containers` lifecycle in application startup and shutdown. | |||
|
|||
NOTE: The framework offers the `DefaultListenerContainerRegistry` implementation, where you can override the `SmartLifecycle.phase`. By default, the `phase` value is `MessageListenerContainer.DEFAULT_PHASE` |
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 try not to use you
in the documentation, and adopt a more formal / neutral tone.
How about something along these lines:
NOTE: The framework offers the `DefaultListenerContainerRegistry` implementation, where you can override the `SmartLifecycle.phase`. By default, the `phase` value is `MessageListenerContainer.DEFAULT_PHASE` | |
NOTE: The `DefaultListenerContainerRegistry ` implementation provided by the framework allows the phase value to be set through the `setPhase` method. The default value is `MessageListenerContainer.DEFAULT_PHASE`. |
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.
Done
docs/src/main/asciidoc/sqs.adoc
Outdated
@@ -955,6 +957,8 @@ MessageListenerContainer<Object> listenerContainer(SqsAsyncClient sqsAsyncClient | |||
} | |||
---- | |||
|
|||
NOTE: You can also specify the `SmartLifecycle.phase`, using `SqsMessageListenerContainer.builder()`, if you are interested on overriding the default value defined in `MessageListenerContainer.DEFAULT_PHASE` |
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.
Same comment here about using you
. Please try to rewrite using the passive voice.
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.
Done
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.
Looks great, thanks @estigma88! Looking forward to more!
📢 Type of change
📜 Description
SmartLifecycle.phase
onSqsMessageListenerContainer
using the builderSmartLifecycle.phase
onAbstractMessageListenerContainer
using thesetPhase
methodSmartLifecycle.phase
onDefaultListenerContainerRegistry
using thesetPhase
methodSqsMessageListenerContainerTests
,AbstractMessageListenerContainerTests
andDefaultListenerContainerRegistryTests
💡 Motivation and Context
Details on #693
💚 How did you test it?
SqsMessageListenerContainerTests
andAbstractMessageListenerContainerTests
📝 Checklist
🔮 Next steps