Enable Redis connection pool if commons-pool2 is on the classpath #26326
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
snicoll
added
type: enhancement
...ain/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
...n/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/boot/autoconfigure/data/redis/RedisClientPoolingFailureAnalyzer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the PR @weixsun. I started to review in the aim of preparing this PR for our next feature release but there's too much work so I'd like you to have another look if possible. I suspect that the tests should be revisited as well.
Let me know if you have time. Thanks!
...ain/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
| final boolean isSentinelConfig = (getSentinelConfig() != null); | ||
| // Jedis Sentinel cannot operate without a pool. | ||
| if (isSentinelConfig) { | ||
| throw new RedisClientPoolingException(ClientType.JEDIS); |
There was a problem hiding this comment.
That looks like something new to me and unrelated to configure the pool automatically. I am happy to consider this case at a later stage but for now please revert.
There was a problem hiding this comment.
I got it. Regarding this, I will report it in another issue or pr.
...ain/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
...n/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
...n/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
...boot-autoconfigure/src/main/resources/META-INF/additional-spring-configuration-metadata.json
Outdated
Show resolved
Hide resolved
spring-boot-project/spring-boot-autoconfigure/src/main/resources/META-INF/spring.factories
Outdated
Show resolved
Hide resolved
...rg/springframework/boot/autoconfigure/data/redis/RedisClientPoolingFailureAnalyzerTests.java
Outdated
Show resolved
Hide resolved
...ng-boot-smoke-test-data-redis/src/main/java/smoketest/data/redis/SampleRedisApplication.java
Outdated
Show resolved
Hide resolved
...ng-boot-smoke-test-data-redis/src/main/java/smoketest/data/redis/SampleRedisApplication.java
Outdated
Show resolved
Hide resolved
snicoll
added
the
status: waiting-for-feedback
First of all, thank you for your review. You have indeed left a lot of suggestions for modification. Maybe it’s time to reach our goal again in an elegant and simple way. The good news is that I have plenty of time now, and I will reorganize and make appropriate modify. |
spring-projects-issues
added
status: feedback-provided
snicoll
added
status: waiting-for-feedback
|
@weixsun those extra commits won't be reviewed as I've already reviewed the PR locally and polished things up. It is interesting though because I more or less did the same on my side. |
snicoll
removed
the
status: waiting-for-feedback
|
@snicoll I got it. Experienced you can always be trusted. I'm sorry that I brought you extra work because of my original excessive thinking and coding.😅 |
Is #26202 concise version
@snicoll
I'm really sorry, I just want to make the commits look less, but it makes things more complicated.
There will be no changes before the merger or approval, please let things go back to normal, sorry again