-
Notifications
You must be signed in to change notification settings - Fork 124
Make redis client pool size configurable via GNMI_REDIS_POOL_SIZE #706
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
Open
make1980
wants to merge
2
commits into
sonic-net:master
Choose a base branch
from
make1980:redis-configurable-pool-size
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // redis_pool.go provides optional, environment-driven tuning of the redis | ||
| // client connection pool used by gNMI's redis clients. | ||
|
|
||
| package dbconfig | ||
|
|
||
| import ( | ||
| "os" | ||
| "strconv" | ||
|
|
||
| log "github.com/golang/glog" | ||
| "github.com/redis/go-redis/v9" | ||
| ) | ||
|
|
||
| // RedisPoolSizeEnvVar is the environment variable used to override the redis | ||
| // client connection pool size. When it is not set, callers must leave the | ||
| // go-redis default pool size unchanged. | ||
| const RedisPoolSizeEnvVar = "GNMI_REDIS_POOL_SIZE" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a |
||
|
|
||
| // ApplyRedisPoolSize sets opts.PoolSize from the GNMI_REDIS_POOL_SIZE | ||
| // environment variable. | ||
| // | ||
| // If the variable is unset, empty, or not a positive integer, opts is left | ||
| // unchanged so go-redis keeps its default pool size (10 * runtime.GOMAXPROCS), | ||
| // which scales with the number of logical CPUs. Setting the variable lets | ||
| // operators cap per-client connections (and their buffers) on high-CPU | ||
| // systems to reduce memory consumption. | ||
| func ApplyRedisPoolSize(opts *redis.Options) { | ||
|
Comment on lines
+19
to
+27
|
||
| if opts == nil { | ||
| return | ||
| } | ||
| v, ok := os.LookupEnv(RedisPoolSizeEnvVar) | ||
| if !ok || v == "" { | ||
| // Not set: do not modify the default pool size. | ||
| return | ||
| } | ||
| n, err := strconv.Atoi(v) | ||
| if err != nil { | ||
| log.Warningf("Ignoring invalid %s=%q (must be a positive integer): %v", | ||
| RedisPoolSizeEnvVar, v, err) | ||
| return | ||
| } | ||
| if n <= 0 { | ||
| log.Warningf("Ignoring non-positive %s=%q (must be a positive integer)", | ||
| RedisPoolSizeEnvVar, v) | ||
| return | ||
| } | ||
| opts.PoolSize = n | ||
| } | ||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package dbconfig | ||
|
|
||
| import ( | ||
| "os" | ||
| "testing" | ||
|
|
||
| "github.com/redis/go-redis/v9" | ||
| ) | ||
|
|
||
| func TestApplyRedisPoolSize(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| set bool | ||
| value string | ||
| expected int // expected PoolSize after apply; 0 means left at default | ||
| }{ | ||
| {name: "unset leaves default", set: false, expected: 0}, | ||
| {name: "empty leaves default", set: true, value: "", expected: 0}, | ||
| {name: "valid positive value applied", set: true, value: "5", expected: 5}, | ||
| {name: "zero leaves default", set: true, value: "0", expected: 0}, | ||
| {name: "negative leaves default", set: true, value: "-3", expected: 0}, | ||
| {name: "non-integer leaves default", set: true, value: "abc", expected: 0}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if tt.set { | ||
| t.Setenv(RedisPoolSizeEnvVar, tt.value) | ||
| } else { | ||
| os.Unsetenv(RedisPoolSizeEnvVar) | ||
| } | ||
|
|
||
| opts := &redis.Options{} | ||
| ApplyRedisPoolSize(opts) | ||
| if opts.PoolSize != tt.expected { | ||
| t.Errorf("PoolSize = %d, want %d", opts.PoolSize, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestApplyRedisPoolSizeNilOptions(t *testing.T) { | ||
| // Must not panic on nil options. | ||
| ApplyRedisPoolSize(nil) | ||
| } |
Oops, something went wrong.
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.
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.
This import breaks the pure build because
sonic_db_configalso pulls inswsscommon. Could we move the existing implementation and test to a pure package like:Then add
internal/redisoptstoPURE_PACKAGESinpure.mkand update the call sites to: