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

meta/sql: more connection control options #5512

Merged
merged 4 commits into from
Jan 8, 2025
Merged

Conversation

anysql
Copy link
Contributor

@anysql anysql commented Jan 6, 2025

Add four new options for sql meta engine

1, max-open-conns, limit the maximum connection, default 0 (unlimited)
2, max-idle-conns, limit the maximum idle connection, previous is cpu number * 2, the new default is 10
3, max-idle-time, previous is 5 mins, no changed, just expose the option.
4, max-life-time, if a connection has be created more than given time, it will be closed anyway, default is 3600, as a backend connection may have some unclean resources.

@jiefenghuang
Copy link
Contributor

Redis and KV have similar configurations, perhaps can change to a handling method similar to redis.ParseURL

@anysql
Copy link
Contributor Author

anysql commented Jan 6, 2025

Redis and KV have similar configurations, perhaps can change to a handling method similar to redis.ParseURL

Got.

@anysql
Copy link
Contributor Author

anysql commented Jan 6, 2025

Redis connection pool is provided by the driver, you can control the settings by meta url, we just need to make it clear in docuemnt.

While sql's connection pool is provided by the database/sql module, not by driver.

for redis/tikv: https://github.com/redis/go-redis/blob/91dddc2e1108c779e8c5b85fd667029873c95172/options.go#L451 setupConnParams function

@zhijian-pro
Copy link
Contributor

These parameters are not universal, so it is more appropriate to use query parameters in the meta-url, which may be supported natively by the driver library.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 48.14815% with 28 lines in your changes missing coverage. Please review.

Project coverage is 54.92%. Comparing base (beaa0d0) to head (8794f76).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
pkg/meta/sql.go 48.14% 20 Missing and 8 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5512      +/-   ##
==========================================
- Coverage   55.09%   54.92%   -0.17%     
==========================================
  Files         166      165       -1     
  Lines       47787    47929     +142     
==========================================
  Hits        26327    26327              
- Misses      18613    18732     +119     
- Partials     2847     2870      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anysql
Copy link
Contributor Author

anysql commented Jan 8, 2025

A better default setting may be "max_open_conns=30&max_idle_conns=30&max_idle_time=300&max_life_time=3600".

Let the max_idle_conns >= max_open_conns can eliminate the short connections effectively

@jiefenghuang jiefenghuang changed the title add more connection options to sql meta engine meta/sql: support more connection options Jan 8, 2025
@jiefenghuang jiefenghuang merged commit d398f8b into main Jan 8, 2025
35 checks passed
@jiefenghuang jiefenghuang deleted the sql_more_db_options branch January 8, 2025 08:54
@anysql anysql changed the title meta/sql: support more connection options meta/sql: more connection control options Jan 8, 2025
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