perf(redis): tune client pool config - #24
Conversation
|
| Filename | Overview |
|---|---|
| internal/kv/redis/redis.go | Signature of New/NewStore changed from string to config.KV; new applyOptions helper unconditionally overwrites all timeout/pool fields after ParseURL, which silently discards any values already set by URL query parameters. |
| internal/config/config.go | Adds RedisPool, RedisTimeouts, RedisConnMaxIdle, and RedisDisableIdentity fields with env-var parsing; defaults are reasonable except RedisDisableIdentity defaults to true, inverting go-redis's own default. |
| internal/config/config_test.go | Good coverage: adds default checks, override checks, and a parse-error accumulation check for the new Redis config fields. |
| internal/kv/redis/redis_test.go | New white-box test for applyOptions verifies all fields are mapped correctly from config.KV to goredis.Options. |
| internal/httpserver/server.go | One-line call-site update from kvredis.New(cfg.KV.RedisURL) to kvredis.New(cfg.KV); no logic change. |
| internal/mcpserver/server.go | Identical one-line call-site update as httpserver; no logic change. |
| README.md | Documents all ten new KV_REDIS_* environment variables with their example values; accurate and complete. |
Sequence Diagram
sequenceDiagram
participant Env as Environment
participant Config as config.Read
participant Redis as kvredis.New
participant ParseURL as goredis.ParseURL
participant ApplyOpts as applyOptions
participant Client as goredis.NewClient
Env->>Config: "KV_REDIS_URL and KV_REDIS_* vars"
Config-->>Redis: config.KV struct
Redis->>ParseURL: cfg.RedisURL
ParseURL-->>Redis: goredis.Options with Addr, DB, creds, and any URL query params
Redis->>ApplyOpts: opts and cfg
Note over ApplyOpts: Overwrites PoolSize, Timeouts,<br/>IdleConns, DisableIdentity<br/>env-var values win over URL params
ApplyOpts-->>Redis: modified opts
Redis->>Client: opts
Client-->>Redis: goredis.Client
Reviews (1): Last reviewed commit: "perf(redis): tune client pool config" | Re-trigger Greptile
| if err != nil { | ||
| return nil, err | ||
| } | ||
| applyOptions(opts, cfg) |
There was a problem hiding this comment.
URL query-param options silently overridden
applyOptions unconditionally overwrites every timeout and pool field that goredis.ParseURL may have already set from URL query parameters. go-redis ParseURL explicitly supports parameters such as ?read_timeout=2s&pool_size=10&dial_timeout=3 (see options.go). Any user whose KV_REDIS_URL includes those query params will have them silently discarded and replaced by the env-var defaults after this change — e.g., redis://host:6379/0?read_timeout=5s would have ReadTimeout set to 5s by ParseURL, then immediately overwritten to 750ms by applyOptions. Guard the assignment so URL-derived non-zero values are not replaced by defaults, or document that URL query params are unsupported.
| Write: p.duration("KV_REDIS_WRITE_TIMEOUT", 750*time.Millisecond), | ||
| }, | ||
| RedisConnMaxIdle: p.duration("KV_REDIS_CONN_MAX_IDLE_TIME", 10*time.Minute), | ||
| RedisDisableIdentity: p.boolean("KV_REDIS_DISABLE_IDENTITY", true), |
There was a problem hiding this comment.
RedisDisableIdentity defaults to true, changing existing behavior
Before this PR, no DisableIdentity override existed, so go-redis defaulted to false (identity enabled). Setting the default to true silently disables the CLIENT SETNAME / CLIENT INFO handshake on every new connection for all existing deployments that don't set KV_REDIS_DISABLE_IDENTITY. This makes Redis-side connection monitoring invisible by default. Defaulting to false (the go-redis default) preserves backward-compatible behavior; operators who want to suppress identity for performance can opt-in explicitly.
| RedisDisableIdentity: p.boolean("KV_REDIS_DISABLE_IDENTITY", true), | |
| RedisDisableIdentity: p.boolean("KV_REDIS_DISABLE_IDENTITY", false), |
KV_REDIS_*settings for redis pool sizing, timeouts, idle lifetime, and identity suppressionconfig.KVvalues when constructing the go-redis client for both http and stdio serversREADME.md