-
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
Signed-off-by: rickbrouwer <[email protected]>
…estart (kedacore#6322) Signed-off-by: Jorge Turrado <[email protected]>
* fix: ensure consistent JSON log format for automaxprocs Signed-off-by: Omer Aplatony <[email protected]> * moved to Unreleased Signed-off-by: Omer Aplatony <[email protected]> --------- Signed-off-by: Omer Aplatony <[email protected]>
* feat: add nsq scaler Signed-off-by: Matt Ulmer <[email protected]> * fix changelog and image ref Signed-off-by: Matt Ulmer <[email protected]> * fix changelog formatting Signed-off-by: Matt Ulmer <[email protected]> * fix: address comments Signed-off-by: Matt Ulmer <[email protected]> * feat: add useHttps/unsafeSsl to nsqScaler Signed-off-by: Matt Ulmer <[email protected]> * fix: make nsq e2e tests more reliable Signed-off-by: Matt Ulmer <[email protected]> * Update tests/scalers/nsq/nsq_test.go Co-authored-by: Jorge Turrado Ferrero <[email protected]> Signed-off-by: Matt Ulmer <[email protected]> * fix: update changelog Signed-off-by: Matt Ulmer <[email protected]> --------- Signed-off-by: Matt Ulmer <[email protected]> Co-authored-by: Jorge Turrado Ferrero <[email protected]>
since code was missing for setting a scaledjob as ready it was stuck as unready if there ever was a problem This is a fix for a regression in kedacore#5916 Signed-off-by: Mårten Svantesson <[email protected]>
Signed-off-by: rickbrouwer <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
* Refactor AWS DynamoDB Streams scaler configuration Signed-off-by: Omer Aplatony <[email protected]> * fixed unit tests Signed-off-by: Omer Aplatony <[email protected]> * Fix invalid value test Signed-off-by: Omer Aplatony <[email protected]> * go fmt Signed-off-by: Omer Aplatony <[email protected]> --------- Signed-off-by: Omer Aplatony <[email protected]>
Adds a global connection pool for MySQL, supporting #6314
Checklist
Fixes #6314
Relates to #