-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
add global connection pool for mysql #6327
base: main
Are you sure you want to change the base?
Conversation
218e3c9
to
52f5ef4
Compare
Signed-off-by: Rob Pickerill <[email protected]>
52f5ef4
to
79304e9
Compare
pkg/scalers/mysql_scaler.go
Outdated
// setConnectionPoolConfiguration configures the MySQL connection pool settings | ||
// based on the parameters provided in mySQLMetadata. If a setting is zero, it | ||
// is left at its default value. | ||
func setConnectionPoolConfiguration(meta *mySQLMetadata, db *sql.DB) { |
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.
TODO: Reconcile these values when scalers share pools but present different connection pool settings
- select max/min/etc
- OR create new connection and add to pool
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.
I think that maybe we could hide this parameter for the moment and share them if there is any request from users. As they can have different values, this could be so difficult to debug.
@kedacore/keda-core-contributors ?
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.
Hey, yeah, just to clarify, did you mean to remove all the new parameters, thus making the globally shared connection pools the default, or just the connection pool tuning parameters?
I also thought a MySQL scaler v2 might fit as well for this, if that's of interest at all?
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.
I'm not sure if adding a v2 scaler makes sense as we could potentially implement a connection pool as part of #6360.
Definitivelly, supporting this in mysql/postgre scalers is worth, but adding v2 scaler to include this feature could be an overkill as it won't make sense if we address a global connection pool
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.
Sure, was just a musing thought, and I lean on your thoughts here as you know the project much better than I
I was exploring how I might even end to end test this so I'll poke at that a bit more then move it out of draft when its all done. I'm also not entirely convinced by the global scope of the connection pool cache.
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.
@kedacore/keda-core-contributors ?
4861916
to
886467d
Compare
Signed-off-by: Rob Pickerill <[email protected]>
886467d
to
c0e7abf
Compare
Signed-off-by: Rob Pickerill <[email protected]>
c0e7abf
to
c988e5e
Compare
Adds a global connection pool for MySQL, supporting #6314
Checklist
Fixes #6314
Relates to #