Deprecated 'idleConnectionPoolSize' config option #432
Conversation
zhenlineo
left a comment
zhenlineo
left a comment
There was a problem hiding this comment.
Wonder if it makes sense to not mix maxIdle and max size, as they are kind of different in meanings?
| * Max number of idle connections per URL for this driver. | ||
| * | ||
| * @return the max number of connections | ||
| * @deprecated please use {@link #maxConnectionPoolSize()} instead. |
There was a problem hiding this comment.
I would suggest changing to Please consider to use #maxConnectionPoolSize and #MaxConnectionLifetime to replace this method instead.
Same applies for comments of all MaxIdle methods
There was a problem hiding this comment.
Improved javadoc with your suggestion for withMaxSessions, withMaxIdleSessions, and withMaxIdleConnections.
| { | ||
| this.maxIdleConnectionPoolSize = size; | ||
| return this; | ||
| return withMaxConnectionPoolSize( size ); |
There was a problem hiding this comment.
I was about to suggest to give a warning here, but then realized we might do not have a logger till this moment...
I feel we should not delegate maxIdle to max, as this config changes meaning already.
I am wondering if it makes sense to just ignore the value we set to this method. What do you think of the following changes?
MaxPoolSize -> MaxConnectionPoolSize
MaxIdleConnections -> ignored
MaxIdleSessions -> ignored
There was a problem hiding this comment.
Since we do not have any idle connection checking in place on the async implementation, ignoring MaxIdleConnections and MaxIdleSessions config values makes more sense for me.
There was a problem hiding this comment.
Thanks for pointing this out. I agree, will change
|
@zhenlineo, @ali-ince suggested config change implemented |
lutovich
force-pushed
the
1.5-deprecate-idleConnectionPoolSize
branch
from
54569ed to
e0ed529
Compare
Newly added 'maxConnectionPoolSize' option should be used instead.
These two config settings will now be ignored and not delegate to `maxConnectionPoolSize`. This feels better because logic of enforcing "max idle" is now gone.
lutovich
force-pushed
the
1.5-deprecate-idleConnectionPoolSize
branch
from
e0ed529 to
b8db2f0
Compare
3 participants
Newly added 'maxConnectionPoolSize' option should be used instead.