Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

robpickerill
Copy link
Contributor

Adds a global connection pool for MySQL, supporting #6314

Checklist

Fixes #6314

Relates to #

@robpickerill robpickerill requested a review from a team as a code owner November 10, 2024 13:27
@robpickerill robpickerill force-pushed the global-mysql-conn-pool branch 6 times, most recently from 218e3c9 to 52f5ef4 Compare November 10, 2024 16:48
@robpickerill robpickerill force-pushed the global-mysql-conn-pool branch from 52f5ef4 to 79304e9 Compare November 10, 2024 17:00
// 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) {
Copy link
Contributor Author

@robpickerill robpickerill Nov 10, 2024

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

Copy link
Member

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 ?

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

@robpickerill robpickerill Nov 25, 2024

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kedacore/keda-core-contributors ?

@robpickerill robpickerill force-pushed the global-mysql-conn-pool branch from 4861916 to 886467d Compare November 24, 2024 15:36
@robpickerill robpickerill force-pushed the global-mysql-conn-pool branch from 886467d to c0e7abf Compare November 24, 2024 15:37
@robpickerill robpickerill force-pushed the global-mysql-conn-pool branch from c0e7abf to c988e5e Compare November 24, 2024 15:50
rickbrouwer and others added 9 commits November 30, 2024 18:41
* 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]>
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement connectionPool into mysql scaler based on grpc connectionPool
7 participants