-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Allow configuring phase in RedisMessageListenerContainer
#3224
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
Conversation
…in RedisMessageListenerContainer. Signed-off-by: Su Ko <[email protected]>
8143666
to
982b789
Compare
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 for the pull request. Care to also add a field for the autoStartup
property? That would nicely align with JedisConnectionFactory
and LettuceConnectionFactory
.
RedisMessageListenerContainer
Done ! 🙂 |
…isMessageListenerContainer Signed-off-by: Su Ko <[email protected]>
f9c2019
to
5ae169b
Compare
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.
👋🏻 @bandalgomsu ,
Thank you for this excellent contribution! It is looking good.
Do you mind adding the same treatment to the org.springframework.data.redis.stream.DefaultStreamMessageListenerContainer
as it also adheres to the SmartLifecycle
contract and will likely benefit from this improvement?
Thanks
src/main/java/org/springframework/data/redis/listener/RedisMessageListenerContainer.java
Show resolved
Hide resolved
Hi @onobc What do you think about allowing 'phase' and 'autoStartup' to be configured either on 'StreamMessageListenerContainer' or through 'StreamMessageListenerContainerOptions' ? ? |
2bd17c9
to
0e59f19
Compare
|
Agreed. Good suggestion @bandalgomsu . |
f5d7dd2
to
396711e
Compare
Signed-off-by: Su Ko <[email protected]>
…hase' in `DefaultStreamMessageListenerContainer` Signed-off-by: Su Ko <[email protected]>
…hase' in `StreamMessageListenerContainerOptions` Signed-off-by: Su Ko <[email protected]>
0e59f19
to
5dde5b2
Compare
The implementation has been completed. Could you review it ?? 🙂 |
src/main/java/org/springframework/data/redis/stream/DefaultStreamMessageListenerContainer.java
Show resolved
Hide resolved
This commit allows configuration of the `phase` and `autoStartup` lifecycle attributes on the `RedisMessageListenerContainer` and `DefaultStreamMessageListenerContainer`. Original Pull Request: #3224 Resolves: #3208 Signed-off-by: Su Ko <[email protected]>
Removes the lifecycle setters from the stream container as it should be configured via the stream options. Also rewords the javadoc comments and moves the autoStartup get/set by the phase get/set. Original Pull Request: #3224 Related tickets: #3208 Signed-off-by: Chris Bono <[email protected]>
Thank you @bandalgomsu for the excellent enhancement - much appreciated. I added a minor polish commit here and am closing this PR in favor of c88d6b1 |
This PR introduces a configurable
phase
property inRedisMessageListenerContainer
.Previously, the container always used
Integer.MAX_VALUE
as its SmartLifecycle phase, making it difficult to align startup/shutdown ordering with other SmartLifecycle beans.#3208