Redis pooling minIdle property is ignored as timeBetweenEvictionRun defaults to -1 and cannot be configured #16703
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
wilkinsona
changed the title
min-idle config does not work
wilkinsona
added
type: bug
wilkinsona
left a comment
wilkinsona
left a comment
There was a problem hiding this comment.
Thanks very much for opening your first Spring Boot PR, @geminiKim. I've left a few comments suggesting some minor improvements that I think could be made.
| config.setMaxTotal(properties.getMaxActive()); | ||
| config.setMaxIdle(properties.getMaxIdle()); | ||
| config.setMinIdle(properties.getMinIdle()); | ||
| config.setTimeBetweenEvictionRunsMillis( |
There was a problem hiding this comment.
Jedis supports the same configuration property. Would you like to make the equivalent change there as well?
There was a problem hiding this comment.
I think it does not the responsibility of Spring-Boot.
Spring-Boot configured the Jedis-Pool using the below code:
So, I think JedisPoolConfig is the responsibility of Jedis.
JedisPoolConfig sets the default value of TimeBetweenEvictionRunsMillis
So, I'm not sure I should handle this equally.
I will wait for your opinion, Thank you for your fast feedback and thoughtful review :)
There was a problem hiding this comment.
Thanks. I'd missed that JedisPoolConfig sets the property already and, therefore, has a different default to Lettuce.
Unfortunately, this creates a problem as the Pool class is shared between Jedis and Lettuce configuration properties. As currently proposed here, the changes will result in a spring.redis.jedis.pool.time-between-eviction-runs property that has no effect. We need to figure out how to avoid that while also coping with the different defaults.
I think our best option is to declare the timeBetweenEvictionRuns property with a null default value and only map it onto the Lettuce or Jedis pool config if the value is non-null.
There was a problem hiding this comment.
I agree that it seems to be the best way now, I modified it.
Thank you for your clear and good feedback. 😄
| assertThat(poolConfig.getMaxIdle()).isEqualTo(4); | ||
| assertThat(poolConfig.getMaxTotal()).isEqualTo(16); | ||
| assertThat(poolConfig.getMaxWaitMillis()).isEqualTo(2000); | ||
| assertThat(poolConfig.getTimeBetweenEvictionRunsMillis()) |
There was a problem hiding this comment.
Would you like to make an equivalent change to the tests for configuring Jedis as well?
There was a problem hiding this comment.
I made an equivalent change to the tests for configuring Jedis.
| * When positive, the idle object evictor thread starts. When non-positive, no | ||
| * idle object evictor thread runs. | ||
| */ | ||
| private Duration idleEvictPeriod = Duration.ofMillis(-1); |
There was a problem hiding this comment.
I think this name should align more closely with the setting that it maps to. Something like timeBetweenEvictionRuns, perhaps.
There was a problem hiding this comment.
I totally agree with your opinion. 👍
| private Duration maxWait = Duration.ofMillis(-1); | ||
|
|
||
| /** | ||
| * Time of milliseconds to sleep between runs of the idle object evictor thread. |
There was a problem hiding this comment.
As with the existing description for maxWait, the description shouldn't mention the unit (milliseconds in this case). Using Duration means that you can specify the time in any unit you want.
|
@wilkinsona I apologize for the mention. And failing tests seem to fail for other reasons, I only worked |
|
No need to apologise. There's nothing more for you to do, I'm just a bit snowed under preparing for next week's Spring IO conference. |
|
Thank you for your quick feedback during the busy time. :D |
|
@geminiKim Thank you very much for making your first contribution to Spring Boot. The proposed changes have been merged into 2.1.x and master along with a small polishing commit. |
|
@wilkinsona Thank you for your kind review in busy times. I will continue to contribute. And i will cheer your session on Spring I/O. :D |
3 participants
Hi :D
I found the bug that
min-idledoes not work when configure Lettuce with pool config.You can find out why the problem occurred by looking below link.
https://commons.apache.org/GenericObjectPool.html#setMinIdle
TimeBetweenEvictionRunsMillisneed to configured greater then zero for themin-idleto working correctly.Some users can trust settings and use them incorrectly in an environment that does not work properly.
So, I added that property, but I don't know if this is a perfectly good solution.
Please review and let me know what you think about this issue.
Thank you :)