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

fix: close mysql connection after create database #366

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

FabianKramm
Copy link
Contributor

@FabianKramm FabianKramm commented Nov 12, 2024

One problem we encountered when using Kine with the MySQL driver is that it uses an unexpected high amount of database connections, even after setting the flag --datastore-max-open-connections to a small number.

We investigated and found out that during the createDBIfNotExist function two database connections are created as well that are never closed. For the PostgreSQL driver the connection is closed as expected:

defer db.Close()

For the MySQL driver, these close after creation defers are missing and hence the connections stay open all the time. This PR adds two lines to close the database connections as expected after making sure the database exists in the MySQL server.

EDIT: Not sure why the drone ci build is failing, but it seems unrelated to this change

@brandond
Copy link
Member

it looks like revive picked up some new nits. I can fix those tomorrow and you can rebase, or you're welcome to add a second commit that ignores the redefines-builtin-id check for now if you're up for it.

@FabianKramm FabianKramm force-pushed the fix-database-connection branch 5 times, most recently from b9a6792 to 975279b Compare November 12, 2024 10:06
@FabianKramm
Copy link
Contributor Author

FabianKramm commented Nov 12, 2024

@brandond thanks for the quick approve, I ignored the linter for the 2 functions, tests seem to run now

EDIT: tests passed 🎉

@brandond brandond merged commit 59c88f9 into k3s-io:master Nov 12, 2024
3 checks passed
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.

3 participants