-
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
mp911de
left a 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 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
onobc
left a 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.
👋🏻 @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
phaseproperty inRedisMessageListenerContainer.Previously, the container always used
Integer.MAX_VALUEas its SmartLifecycle phase, making it difficult to align startup/shutdown ordering with other SmartLifecycle beans.#3208