feat: refactor redispool #408
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Good style for options! As my opinion, not all the options from goredis should be revealed such as tls.Config.
#403 and #405 are giving some moderate modification for #398 and #404. Refactor of redispool options's code should be an independent issue to discuss and enhance, which can be merge later. |
andrewshan
reviewed
Jun 5, 2022
common/redispool/redis_pool.go
Outdated
| } | ||
|
|
||
| // DefaultConfig redis pool configuration with default values | ||
| func DefaultConfig() *Config { | ||
| return &Config{ | ||
| MaxIdle: 200, | ||
| IdleTimeout: commontime.Duration(120 * time.Second), | ||
| ConnectTimeout: commontime.Duration(300 * time.Millisecond), |
chuntaojun
reviewed
Jun 5, 2022
| // WithConfig set new config for NewPool,keep old code compatibility | ||
| func WithConfig(newConfig *Config) Option { | ||
| return func(c *Config) { | ||
| c = newConfig |
magederek
reviewed
Jun 5, 2022
common/redispool/redis_pool.go
Outdated
| WithTLS bool `json:"withTLS"` | ||
|
|
||
| // TLS Config to use. When set TLS will be negotiated. | ||
| TlsConfig *tls.Config |
There was a problem hiding this comment.
both tlsConfig itself and fields in tls.Config have not json tag for unmarshalling from JSON config. Currently, only MinVersion field is useful for most of the user, I think we could hide the details of tls config.
|
There is no problem with the design here. You can understand it by looking carefully at the design of with options
…------------------ Original ------------------
From: liaochuntao ***@***.***>
Date: Sun,Jun 5,2022 10:37 PM
To: polarismesh/polaris ***@***.***>
Cc: heige ***@***.***>, Author ***@***.***>
Subject: Re: [polarismesh/polaris] feat: refactor redispool (PR #408)
@chuntaojun commented on this pull request.
In common/redispool/config_option.go:
> +package redispool + +import ( +"crypto/tls" +"time" + +commontime "github.com/polarismesh/polaris-server/common/time" +) + +// Option functional options for Config +type Option func(c *Config) + +// WithConfig set new config for NewPool,keep old code compatibility +func WithConfig(newConfig *Config) Option { +return func(c *Config) { +c = newConfig
这个逻辑，应该是问题的？并不能真正影响c的实际内容吧
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
This is a private attribute. It is provided to the upstream of the business for configuration. It does not need to be exposed here. Moreover, each TLS configuration is optional for users, so it is designed in this way. See with options.
…------------------ Original ------------------
From: Derek.Chen ***@***.***>
Date: Sun,Jun 5,2022 10:39 PM
To: polarismesh/polaris ***@***.***>
Cc: heige ***@***.***>, Author ***@***.***>
Subject: Re: [polarismesh/polaris] feat: refactor redispool (PR #408)
@magederek commented on this pull request.
In common/redispool/redis_pool.go:
> +// Connection age at which client retires (closes) the connection. +// Default is to not close aged connections. +MaxConnAge commontime.Duration `json:"maxConnAge"` + +// Use the specified Username to authenticate the current connection +// with one of the connections defined in the ACL list when connecting +// to a Redis 6.0 instance, or greater, that is using the Redis ACL system. +Username string `json:"username"` + +// WithTLS whether open TLSConfig +// if WithTLS is true, you should call WithEnableWithTLS,and then TLSConfig is not should be nil +// In this case you should call WithTLSConfig func to set tlsConfig +WithTLS bool `json:"withTLS"` + +// TLS Config to use. When set TLS will be negotiated. +TlsConfig *tls.Config
both tlsConfig itself and fields in tls.Config have not json tag for unmarshalling from JSON config.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
The original implementation here is read-write timeout. The bottom layer of the redis component library is 5S. For a slow operation, it is almost tens of milliseconds. Here, refer to the parameter configuration of the bottom layer go redis.
…------------------ Original ------------------
From: andrew shan ***@***.***>
Date: Sun,Jun 5,2022 10:22 PM
To: polarismesh/polaris ***@***.***>
Cc: heige ***@***.***>, Author ***@***.***>
Subject: Re: [polarismesh/polaris] feat: refactor redispool (PR #408)
@andrewshan commented on this pull request.
In common/redispool/redis_pool.go:
> } // DefaultConfig redis pool configuration with default values func DefaultConfig() *Config { return &Config{ MaxIdle: 200, -IdleTimeout: commontime.Duration(120 * time.Second), -ConnectTimeout: commontime.Duration(300 * time.Millisecond),
默认连接时长和消息超时为什么要改成5s？
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Ok
…------------------ Original ------------------
From: andrew shan ***@***.***>
Date: Mon,Jun 6,2022 11:18 AM
To: polarismesh/polaris ***@***.***>
Cc: heige ***@***.***>, Author ***@***.***>
Subject: Re: [polarismesh/polaris] feat: refactor redispool (PR #408)
@andrewshan commented on this pull request.
In common/redispool/redis_pool.go:
> } // DefaultConfig redis pool configuration with default values func DefaultConfig() *Config { return &Config{ MaxIdle: 200, -IdleTimeout: commontime.Duration(120 * time.Second), -ConnectTimeout: commontime.Duration(300 * time.Millisecond),
默认值和心跳场景有关系，都恢复成300ms吧
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
whether there is a need to add certificate configuration? |
chuntaojun
approved these changes
Jun 6, 2022
|
@daheige hello, pls help to resolve the confilcts |
…lt-auth-adjust Feature/daheige/default auth adjust
…tor-StringSliceDeDuplication feat: refactor StringSliceDeDuplication
[PR polarismesh#397] fix config_center uint test failure
feat: add tls and acl options to redis configuration
…size-minidle feat: add redis options of minIdleConns and poolSize
Codecov Report
@@ Coverage Diff @@
## main #408 +/- ##
==========================================
- Coverage 21.58% 21.42% -0.16%
==========================================
Files 180 182 +2
Lines 24588 24934 +346
==========================================
+ Hits 5307 5343 +36
- Misses 18471 18779 +308
- Partials 810 812 +2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
chuntaojun
approved these changes
Jun 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please provide issue(s) of this PR:
refactor #
refactor common/redispool
for #404
for #403
for #398