adapter: split redis.go / redis_compat_commands.go into cohesive files (no behavior change)#956
Merged
Merged
Conversation
…s (no behavior change)
Owner
Author
|
@claude review |
|
Claude finished @bootjp's task in 2m 12s —— View job Code Review:
|
| File | Domain |
|---|---|
redis.go |
Server struct, lifecycle, dispatch, tracing, pub/sub infra |
redis_txn.go |
MULTI/EXEC, txnContext, dedup machinery |
redis_lists.go |
List push/pop/range/trim (both directions, dedup path) |
redis_strings.go |
SET/GET/DEL/EXISTS options and execution |
redis_keys.go |
KEYS/SCAN pattern matching, visible-key encoding |
redis_proxy_leader.go |
Leader forwarding, go-redis writers |
redis_expire_cmds.go |
SETEX/GETDEL/SETNX, TTL/EXPIRE family |
redis_server_cmds.go |
INFO/CLIENT/COMMAND/HELLO/SELECT/PUBLISH/FLUSH/PUBSUB |
redis_set_cmds.go |
SADD/SREM/SISMEMBER/SMEMBERS/PFADD/PFCOUNT |
redis_hash_cmds.go |
HSET/HGET/HDEL/HEXISTS/HLEN/HINCRBY/HGETALL/INCR |
redis_zset_cmds.go |
ZADD/ZINCRBY/ZRANGE/ZREM/BZPOPMIN |
redis_stream_cmds.go |
XADD/XTRIM/XRANGE/XREVRANGE/XREAD/XLEN |
redis_compat_commands.go |
Shared constants (now misnamed — see §1) |
Verdict
Approve. The refactor is correct and the verification methodology (go doc parity + code-line multiset) is a solid proof of no behavioral change. Items 1–3 above are minor cleanup candidates for a follow-up PR; item 4 is pre-existing and tracked separately. Nothing here warrants blocking the merge.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThirteen new files implement a complete Redis protocol adapter on top of ElasticKV's Raft-backed KV store. The adapter covers strings, lists, hashes, sets, sorted sets, streams, expiry commands, MULTI/EXEC transactions, KEYS/SCAN key discovery, and Raft-leader-aware proxying, all using wide-column storage with legacy-blob migration paths. ChangesRedis Adapter — Full Command Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant RedisServer
participant txnContext
participant Coordinator
participant Store
Client->>RedisServer: MULTI
Client->>RedisServer: SET k v
Client->>RedisServer: RPUSH list a
Client->>RedisServer: EXEC
RedisServer->>RedisServer: verify leader or proxyTransactionToLeader
RedisServer->>txnContext: create with txnStartTS (LastCommitTS+Observe)
loop per queued command
RedisServer->>txnContext: applyXxx (stage dirty values, track read keys)
end
txnContext->>txnContext: validateReadSet (OCC vs LatestCommitTS)
txnContext->>txnContext: prepareDispatch (OperationGroup + fenced commitTS)
txnContext->>Coordinator: commit dispatch
Coordinator-->>RedisServer: ok or retryable error
alt retryable and dedup enabled
RedisServer->>Coordinator: dispatchExecReuse (same write set, new commitTS)
end
RedisServer-->>Client: RESP2 array results
note over Client, Store: Follower read proxying
Client->>RedisServer: GET key (follower node)
RedisServer->>Store: tryLeaderGetAt (versioned RawGet via leader)
Store-->>RedisServer: value or ErrKeyNotFound
RedisServer-->>Client: bulk string reply
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements various Redis command adapters and transaction handlers for elastickv, introducing wide-column storage layouts, fast-path lookups, and transactional optimizations. The review feedback focuses heavily on ensuring proper context propagation by replacing instances of context.Background() with the active request or transaction context (ctx or t.ctx) across multiple command implementations to respect timeouts and cancellations. Additionally, it is recommended to log connection close errors in redis_server_cmds.go instead of silently ignoring them.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var v []byte | ||
| err := r.retryRedisWrite(ctx, func() error { | ||
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), key, readTS) |
Contributor
There was a problem hiding this comment.
The request context ctx is available in this scope but context.Background() is passed to r.keyTypeAt. Using ctx ensures that request timeouts and cancellations are properly respected during the key type lookup.
Suggested change
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | |
| typ, err := r.keyTypeAt(ctx, key, readTS) |
| // Wide-column path: check if any !hs|fld| keys exist for this key. | ||
| hashFieldPrefix := store.HashFieldScanPrefix(key) | ||
| hashFieldEnd := store.PrefixScanEnd(hashFieldPrefix) | ||
| wideKVs, err := r.store.ScanAt(context.Background(), hashFieldPrefix, hashFieldEnd, 1, readTS) |
Contributor
There was a problem hiding this comment.
The request context ctx is available in hdelTxn but context.Background() is passed to r.store.ScanAt. Using ctx ensures that request timeouts and cancellations are properly respected during the scan.
Suggested change
| wideKVs, err := r.store.ScanAt(context.Background(), hashFieldPrefix, hashFieldEnd, 1, readTS) | |
| wideKVs, err := r.store.ScanAt(ctx, hashFieldPrefix, hashFieldEnd, 1, readTS) |
| } | ||
|
|
||
| // Legacy blob path. | ||
| value, err := r.loadHashAt(context.Background(), key, readTS) |
Contributor
There was a problem hiding this comment.
The request context ctx is available in hdelTxn but context.Background() is passed to r.loadHashAt. Using ctx ensures that request timeouts and cancellations are properly respected during the hash load.
Suggested change
| value, err := r.loadHashAt(context.Background(), key, readTS) | |
| value, err := r.loadHashAt(ctx, key, readTS) |
| } | ||
|
|
||
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), key, readTS) |
Contributor
There was a problem hiding this comment.
The request context ctx is available in rangeList but context.Background() is passed to r.keyTypeAt. Using ctx ensures that request timeouts and cancellations are properly respected during the key type lookup.
Suggested change
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | |
| typ, err := r.keyTypeAt(ctx, key, readTS) |
| return nil, errors.WithStack(err) | ||
| } | ||
|
|
||
| meta, exists, err := r.resolveListMeta(context.Background(), key, readTS) |
Contributor
There was a problem hiding this comment.
The request context ctx is available in rangeList but context.Background() is passed to r.resolveListMeta. Using ctx ensures that request timeouts and cancellations are properly respected during the list metadata resolution.
Suggested change
| meta, exists, err := r.resolveListMeta(context.Background(), key, readTS) | |
| meta, exists, err := r.resolveListMeta(ctx, key, readTS) |
| return typ, nil | ||
| } | ||
| t.trackTypeReadKeys(key) | ||
| return t.server.keyTypeAt(context.Background(), key, t.startTS) |
Contributor
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in stagedKeyType. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the key type lookup.
Suggested change
| return t.server.keyTypeAt(context.Background(), key, t.startTS) | |
| ctx := t.ctx | |
| if ctx == nil { | |
| ctx = context.Background() | |
| } | |
| return t.server.keyTypeAt(ctx, key, t.startTS) |
| } | ||
|
|
||
| func (t *txnContext) applySet(cmd redcon.Command) (redisResult, error) { | ||
| if isList, err := t.server.isListKeyAt(context.Background(), cmd.Args[1], t.startTS); err != nil { |
Contributor
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in applySet. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the list check.
ctx := t.ctx
if ctx == nil {
ctx = context.Background()
}
if isList, err := t.server.isListKeyAt(ctx, cmd.Args[1], t.startTS); err != nil {| state.value = &expireAt | ||
| state.dirty = true | ||
| if typ == redisTypeString { | ||
| plain, err := t.server.isPlainRedisString(context.Background(), key, t.startTS) |
Contributor
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in applyPositiveExpire. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the plain string check.
ctx := t.ctx
if ctx == nil {
ctx = context.Background()
}
plain, err := t.server.isPlainRedisString(ctx, key, t.startTS)
Comment on lines
+778
to
+784
| switch { | ||
| case e < persistedLen: | ||
| return t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(e), t.startTS) | ||
| case s >= persistedLen: | ||
| return appendValues(st.appends, s-persistedLen, e-persistedLen), nil | ||
| default: | ||
| head, err := t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(persistedLen-1), t.startTS) |
Contributor
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in listRangeValues. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the list range fetch.
Suggested change
| switch { | |
| case e < persistedLen: | |
| return t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(e), t.startTS) | |
| case s >= persistedLen: | |
| return appendValues(st.appends, s-persistedLen, e-persistedLen), nil | |
| default: | |
| head, err := t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(persistedLen-1), t.startTS) | |
| ctx := t.ctx | |
| if ctx == nil { | |
| ctx = context.Background() | |
| } | |
| switch { | |
| case e < persistedLen: | |
| return t.server.fetchListRange(ctx, key, st.meta, int64(s), int64(e), t.startTS) | |
| case s >= persistedLen: | |
| return appendValues(st.appends, s-persistedLen, e-persistedLen), nil | |
| default: | |
| head, err := t.server.fetchListRange(ctx, key, st.meta, int64(s), int64(persistedLen-1), t.startTS) |
|
|
||
| func (r *RedisServer) quit(conn redcon.Conn, _ redcon.Command) { | ||
| conn.WriteString("OK") | ||
| _ = conn.Close() |
Contributor
There was a problem hiding this comment.
The error returned by conn.Close() is silently ignored. Network connection close errors should be logged to ensure resource leaks or other cleanup problems are visible.
if err := conn.Close(); err != nil {
log.Printf("failed to close connection: %v", err)
}References
- Do not silently ignore errors from
Close()methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
Owner
Author
|
@codex review |
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
Author
|
@claude review |
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
Resolved the merge conflicts by merging |
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
adapter/redis_keys.go-205-237 (1)
205-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPattern matching allows prefix/suffix overlap, causing false positives.
The pattern
a*aincorrectly matches keya. After consuming the prefix"a"(pos=1), the suffix checkHasSuffix(key, "a")passes because the entire key satisfies it, but the prefix and suffix share the same character. Redis glob semantics require*to match zero-or-more characters between distinct prefix and suffix portions.Proposed fix
last := parts[len(parts)-1] - if len(last) > 0 && !bytes.HasSuffix(key, last) { + if len(last) > 0 { + if len(key) < pos+len(last) || !bytes.HasSuffix(key, last) { + return false + } + } - return false - } return true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_keys.go` around lines 205 - 237, The matchesAsteriskPattern function has a bug where the suffix check using HasSuffix does not account for prefix/suffix overlap. When checking if the key ends with the suffix portion (the last part after splitting by asterisk), add a validation that the suffix does not overlap with the already-consumed prefix position. Specifically, ensure that before calling HasSuffix for the last part, verify that the position where the suffix would need to start (len(key) - len(last)) is greater than or equal to the current position (pos) to prevent patterns like "a*a" from incorrectly matching keys like "a" where the prefix and suffix overlap.adapter/redis_set_cmds.go-577-577 (1)
577-577:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate
ctxtoloadSetAtinstead ofcontext.Background().The outer context has a timeout but this call ignores it.
Suggested fix
- value, err := r.loadSetAt(context.Background(), hllKind, cmd.Args[1], readTS) + value, err := r.loadSetAt(ctx, hllKind, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` at line 577, The loadSetAt function call is using context.Background() which ignores any timeout settings from the outer context. Replace context.Background() with ctx in the loadSetAt call to properly propagate the outer context's timeout and cancellation behavior through the function call.adapter/redis_set_cmds.go-461-484 (1)
461-484:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd timeout to context in
sismember.Other command handlers use
context.WithTimeout(e.g., line 451, 568). This handler usescontext.Background()without a timeout, which could cause unbounded blocking on store operations.Suggested fix
func (r *RedisServer) sismember(conn redcon.Conn, cmd redcon.Command) { if r.proxyToLeader(conn, cmd, cmd.Args[1]) { return } key := cmd.Args[1] member := cmd.Args[2] readTS := r.readTS() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + defer cancel() hit, alive, err := r.setMemberFastExists(ctx, key, member, readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 461 - 484, The sismember function uses context.Background() without a timeout, which could cause unbounded blocking on store operations, while other command handlers in the codebase use context.WithTimeout. Replace the ctx initialization in the sismember function to use context.WithTimeout instead of context.Background(), applying the same timeout pattern used by other command handlers (referenced at lines 451 and 568). This ensures the context passed to setMemberFastExists and sismemberSlow methods has appropriate timeout protection.adapter/redis_set_cmds.go-534-562 (1)
534-562:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd timeout context to
smembers.This handler makes multiple store calls using
context.Background()without a timeout. For consistency with write handlers and to prevent unbounded blocking, use a timeout-scoped context.Suggested fix
func (r *RedisServer) smembers(conn redcon.Conn, cmd redcon.Command) { if r.proxyToLeader(conn, cmd, cmd.Args[1]) { return } + ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + defer cancel() readTS := r.readTS() - typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS) + typ, err := r.keyTypeAt(ctx, cmd.Args[1], readTS) if err != nil { writeRedisError(conn, err) return } ... - value, err := r.loadSetAt(context.Background(), setKind, cmd.Args[1], readTS) + value, err := r.loadSetAt(ctx, setKind, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 534 - 562, The smembers function uses context.Background() without a timeout in both the r.keyTypeAt and r.loadSetAt calls, which can cause unbounded blocking. Create a timeout-scoped context at the beginning of the smembers function (following the pattern used in write handlers) and replace both context.Background() calls with this timeout context to ensure consistency and prevent unbounded blocking operations.adapter/redis_set_cmds.go-30-34 (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate context instead of using
context.Background().This function is called from within retry loops that have timeout-scoped contexts. Using
context.Background()ignores cancellation signals and timeouts.Suggested fix
-func (r *RedisServer) validateExactSetKind(kind string, key []byte, readTS uint64) error { - typ, err := r.keyTypeAt(context.Background(), key, readTS) +func (r *RedisServer) validateExactSetKind(ctx context.Context, kind string, key []byte, readTS uint64) error { + typ, err := r.keyTypeAt(ctx, key, readTS)Then update callers (lines 186, 211, 573) to pass
ctx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 30 - 34, The validateExactSetKind function is using context.Background() which ignores timeout and cancellation signals from parent contexts in retry loops. Add a context parameter to the validateExactSetKind function signature, replace the context.Background() call with this new parameter when calling r.keyTypeAt(), and then update all three callers of validateExactSetKind (referenced at lines 186, 211, and 573) to pass their own context instead of letting the function create a new one.adapter/redis_expire_cmds.go-222-223 (1)
222-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.Same issue as other handlers in this file.
Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 222 - 223, The context creation at lines 222-223 uses context.Background() which does not propagate shutdown signals properly. Replace the context.WithTimeout call that uses context.Background() with a call that uses r.handlerContext() instead, while maintaining the same timeout duration (redisDispatchTimeout). This ensures the context respects the handler's lifecycle and proper shutdown propagation similar to other handlers in this file.adapter/redis_expire_cmds.go-94-95 (1)
94-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.Same issue as other handlers in this file.
Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 94 - 95, The context is being created using context.WithTimeout(context.Background(), redisDispatchTimeout) which does not properly propagate shutdowns. Replace this context creation with r.handlerContext() which is the correct method used by other handlers in this file to ensure proper shutdown propagation and context lifecycle management.adapter/redis_expire_cmds.go-30-31 (1)
30-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.
context.Background()is used here, but other handlers (e.g.,setLegacyin redis_strings.go:265) derive context fromr.handlerContext(). This ensures in-flight requests cancel when the server shuts down.Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 30 - 31, Replace context.Background() with r.handlerContext() in the context creation at the beginning of the handler to ensure proper shutdown propagation. This aligns with the pattern used in other handlers like setLegacy and ensures in-flight requests are properly cancelled when the server shuts down. The ctx variable will then derive from the handler's context instead of being a detached background context.adapter/redis_expire_cmds.go-180-186 (1)
180-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
cockerrors.Wrapinstead offmt.Errorffor error wrapping.Per coding guidelines, errors at boundaries should use
github.com/cockroachdb/errorsfor proper stack traces.Suggested fix
func parseExpireTTL(raw []byte) (int64, error) { ttl, err := strconv.ParseInt(string(raw), 10, 64) if err != nil { - return 0, fmt.Errorf("parse expire ttl: %w", err) + return 0, cockerrors.Wrap(err, "parse expire ttl") } return ttl, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 180 - 186, In the parseExpireTTL function, replace the fmt.Errorf call with cockerrors.Wrap to ensure proper stack trace handling according to coding guidelines. Import the cockerrors package from github.com/cockroachdb/errors if not already imported, and use cockerrors.Wrap to wrap the error instead of fmt.Errorf with the %w verb.Source: Coding guidelines
adapter/redis_expire_cmds.go-46-47 (1)
46-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.Same issue as
setex- should derive context fromr.handlerContext()for consistency and proper shutdown behavior.Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 46 - 47, The context initialization at lines 46-47 is creating a new context from context.Background() instead of using the handler's context for proper shutdown propagation. Replace the context.WithTimeout call to derive from r.handlerContext() instead of context.Background(), ensuring consistency with the setex implementation and allowing proper shutdown behavior throughout the handler.adapter/redis_expire_cmds.go-3-16 (1)
3-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMixed error package usage violates coding guidelines.
The file imports both standard
errors(line 5) andcockerrors "github.com/cockroachdb/errors"(line 14). Per coding guidelines, errors at boundaries should consistently usegithub.com/cockroachdb/errorsfor proper stack traces and error wrapping.Line 169 uses
errors.New("ERR syntax error")from the standard library.Suggested fix
import ( "context" - "errors" "fmt" "math" "strconv"And update line 169:
- return false, errors.New("ERR syntax error") + return false, cockerrors.New("ERR syntax error")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 3 - 16, The file has mixed error package imports (standard errors and cockerrors) which violates coding guidelines. Remove the standard `errors` import from the import block and replace the `errors.New("ERR syntax error")` call (referenced in the content at line 169) with `cockerrors.New("ERR syntax error")` to ensure consistent use of the cockroachdb errors package for proper stack traces and error wrapping throughout the file.Source: Coding guidelines
adapter/redis_zset_cmds.go-825-860 (1)
825-860:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
loadZSetAtusescontext.Background()instead of the availablectx.Line 845 calls
loadZSetAt(context.Background(), ...)inside the retry callback wherectx(with timeout) is in scope. This prevents proper cancellation if the parent context times out.- value, _, err := r.loadZSetAt(context.Background(), cmd.Args[1], readTS) + value, _, err := r.loadZSetAt(ctx, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 825 - 860, The zrem method creates a context with timeout using ctx, cancel := context.WithTimeout(...) but then passes context.Background() instead of ctx to the loadZSetAt call on the line that loads the ZSet value. Replace the context.Background() argument in the loadZSetAt call with the ctx variable to ensure proper timeout propagation and cancellation behavior throughout the operation.adapter/redis_zset_cmds.go-862-911 (1)
862-911:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
context.Background()issue aszrem.Line 893 should use
ctxinstead ofcontext.Background()for consistent timeout propagation.- value, _, err := r.loadZSetAt(context.Background(), cmd.Args[1], readTS) + value, _, err := r.loadZSetAt(ctx, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 862 - 911, In the zremrangebyrank function, replace the context.Background() argument in the r.loadZSetAt call with ctx to ensure the timeout created at the start of the function is properly propagated to all operations. The ctx variable with redisDispatchTimeout is already defined earlier in the function and should be used consistently throughout instead of creating a new background context.adapter/redis_zset_cmds.go-920-970 (1)
920-970:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
context.Background()issue inloadZSetAtcall.Line 943 should use
ctxfor timeout propagation.- value, _, err := r.loadZSetAt(context.Background(), key, readTS) + value, _, err := r.loadZSetAt(ctx, key, readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 920 - 970, In the tryBZPopMinWithMode function, the call to r.loadZSetAt is using context.Background() instead of the ctx variable that was created with a timeout at the beginning of the function. Replace context.Background() with ctx in the r.loadZSetAt call to ensure proper timeout propagation throughout the operation.
🧹 Nitpick comments (6)
adapter/redis_server_cmds.go (2)
490-495: ⚡ Quick winUse
slogfor structured logging per coding guidelines.The
log.Printfcall should be replaced withslogfor consistency with the project's structured logging requirements.Suggested fix
func (r *RedisServer) quit(conn redcon.Conn, _ redcon.Command) { conn.WriteString("OK") if err := conn.Close(); err != nil { - log.Printf("redis quit close failed: %v", err) + slog.Error("redis quit close failed", "error", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_server_cmds.go` around lines 490 - 495, In the quit method of the RedisServer type, replace the log.Printf call with slog for structured logging to maintain consistency with the project's logging standards. Use the appropriate slog function (such as slog.Error) to log the error when conn.Close() fails, ensuring the error information is preserved in the structured log output.Source: Coding guidelines
571-577: ⚡ Quick winUse
slogfor trace logging per coding guidelines.Similar to the
quitfunction, trace logging should useslogwith appropriate structured keys.Suggested fix
func (r *RedisServer) publish(conn redcon.Conn, cmd redcon.Command) { count := r.publishCluster(context.Background(), cmd.Args[1], cmd.Args[2]) if r.traceCommands { - log.Printf("redis trace publish remote=%s channel=%q subscribers=%d", conn.RemoteAddr(), string(cmd.Args[1]), count) + slog.Debug("redis trace publish", "remote", conn.RemoteAddr(), "channel", string(cmd.Args[1]), "subscribers", count) } conn.WriteInt64(count) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_server_cmds.go` around lines 571 - 577, In the publish method of the RedisServer type, replace the log.Printf call used for trace logging with structured logging using slog. Extract the trace message components (remote address from conn.RemoteAddr(), channel from cmd.Args[1], and subscriber count) as structured key-value pairs to pass to slog, following the same pattern used in the quit function as referenced in the guidelines.Source: Coding guidelines
adapter/redis_proxy_leader.go (1)
226-245: 💤 Low valueCached Redis clients are never closed, risking connection leaks.
When leadership changes, stale clients for old leader addresses remain in the cache indefinitely. Over time, this can exhaust connection pools or file descriptors. Consider adding a cleanup mechanism (e.g., periodic eviction of unused clients, or replacing the cache entry when a new client is created for the same address after a leadership change).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_proxy_leader.go` around lines 226 - 245, The getOrCreateLeaderClient method caches Redis clients indefinitely without ever closing them, leading to connection leaks when leadership changes. Add a cleanup mechanism to close and remove stale clients from the r.leaderClients cache when they are no longer needed, such as closing the old client before storing a new one for the same address, or implementing a separate method to explicitly close and evict clients from the cache when leadership transitions occur. Ensure the cleanup respects the existing mutex synchronization (r.leaderClientsMu) to prevent race conditions.adapter/redis_stream_cmds.go (1)
1143-1165: ⚡ Quick winRead operations use
context.Background()with no timeout.Multiple store calls (
keyTypeAtExpect,loadStreamMetaAt,loadStreamAt) usecontext.Background(), which means these reads have no timeout and won't abort during graceful shutdown. While less critical than write commands, this is inconsistent withxreadwhich properly derives context fromr.handlerContext().Consider wrapping these in a context with timeout for consistency and to prevent slow reads from hanging indefinitely.
Suggested approach
func (r *RedisServer) xlen(conn redcon.Conn, cmd redcon.Command) { if r.proxyToLeader(conn, cmd, cmd.Args[1]) { return } + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout) + defer cancel() readTS := r.readTS() - typ, err := r.keyTypeAtExpect(context.Background(), cmd.Args[1], readTS, redisTypeStream) + typ, err := r.keyTypeAtExpect(ctx, cmd.Args[1], readTS, redisTypeStream) // ... apply same pattern to other context.Background() calls🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_stream_cmds.go` around lines 1143 - 1165, The read operations in this stream command handler are using context.Background() which provides no timeout for the keyTypeAtExpect, loadStreamMetaAt, and loadStreamAt calls, potentially allowing reads to hang indefinitely and making it inconsistent with how other commands like xread derive their context. Replace all instances of context.Background() in this code block with a context derived from r.handlerContext() to ensure these read operations have proper timeout handling and can be cancelled during graceful shutdown, maintaining consistency with the rest of the codebase.adapter/redis_set_cmds.go (1)
46-52: ⚡ Quick winUse
cockerrors.Wrapfor error wrapping consistency.The coding guidelines require using
github.com/cockroachdb/errorsfor error wrapping. This function usesfmt.Errorfwhile the rest of the file usescockerrors.Suggested fix
func (r *RedisServer) hllExistsAt(key []byte, readTS uint64) (bool, error) { exists, err := r.store.ExistsAt(context.Background(), redisHLLKey(key), readTS) if err != nil { - return false, fmt.Errorf("exists hll: %w", err) + return false, cockerrors.Wrap(err, "exists hll") } return exists, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 46 - 52, In the hllExistsAt function, replace the fmt.Errorf call with cockerrors.Wrap to maintain consistency with error wrapping practices in the rest of the file. The cockerrors.Wrap function should be called with the error returned from r.store.ExistsAt and a descriptive error message like "exists hll" to provide proper error context.Source: Coding guidelines
adapter/redis_zset_cmds.go (1)
792-823: 💤 Low valueConsider propagating a timeout context for read operations.
zrangeReadusescontext.Background()for bothkeyTypeAtExpect(line 794) andloadZSetAt(line 808), unlike write handlers that create timeout contexts. This could cause the operation to hang indefinitely on a slow or unresponsive store.Given that the PR objectives mention context propagation changes, this may warrant verification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 792 - 823, The zrangeRead function uses context.Background() for both the keyTypeAtExpect and loadZSetAt operations, which lacks timeout protection unlike write handlers. Create a timeout context at the beginning of the zrangeRead function (similar to how write handlers handle context creation) and pass that timeout context to both the keyTypeAtExpect call and the loadZSetAt call instead of context.Background() to ensure operations cannot hang indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapter/redis_hash_cmds.go`:
- Around line 848-856: The keyTypeAt function call on line 850 uses
context.Background() instead of the ctx parameter that has the
redisDispatchTimeout configured, which bypasses timeout propagation. Replace
context.Background() with ctx in the keyTypeAt function call to ensure the
timeout context is properly passed through to the store operation.
- Around line 75-150: The functions buildSetLegacyMigrationElems and
buildZSetLegacyMigrationElems are defined in redis_hash_cmds.go but are only
used in redis_set_cmds.go and redis_zset_cmds.go respectively, violating proper
code organization. Move buildSetLegacyMigrationElems to redis_set_cmds.go and
buildZSetLegacyMigrationElems to redis_zset_cmds.go. Additionally, on line 126
where the nolint:mnd directive appears in the capacity calculation for the elems
slice in buildZSetLegacyMigrationElems, replace the //nolint:mnd comment with a
named constant to clarify why 2 operations are needed per entry (one for member
key and one for score index key), and update all references to use this constant
instead of magic numbers.
In `@adapter/redis_keys.go`:
- Around line 14-16: The keys method in the RedisServer type accesses
cmd.Args[1] without validating that the cmd.Args slice contains at least 2
elements, which will cause a panic when a KEYS command is sent without the
required pattern argument. Add a length check before accessing cmd.Args[1] to
verify that len(cmd.Args) >= 2, and if the validation fails, use conn to send an
appropriate error response back to the client indicating that the pattern
argument is missing.
In `@adapter/redis_lists.go`:
- Around line 714-726: The listPushCmd function uses context.Background()
without timeouts in two places, which is inconsistent with other handlers like
lrange and risks indefinite hangs without parent context information. Replace
the context.Background() call in the r.keyTypeAt() method with
context.WithTimeout(r.handlerContext(), redisDispatchTimeout), and similarly
replace the ctx := context.Background() line with the same timeout context
pattern before passing it to pushFn(). This ensures consistency with other
handlers and provides proper timeout and context propagation.
- Around line 811-825: The lindex command handler uses context.Background()
without a timeout in the calls to r.keyTypeAt() and r.listValuesAt() methods,
which can lead to hung operations. Replace both context.Background() calls (at
lines 812 and 825) with a properly timeout-protected context by using
context.WithTimeout(), following the same pattern that was applied to fix
listPushCmd. This ensures that both the type check and value retrieval
operations have proper timeout protection.
In `@adapter/redis_set_cmds.go`:
- Around line 599-632: The pfcount function lacks a timeout-scoped context for
its store operations. After the proxyToLeader check, add a context with timeout
using `ctx, cancel := context.WithTimeout(context.Background(),
redisDispatchTimeout)` and defer the cancel call (following the pattern used in
pfadd), then replace all `context.Background()` calls within the loop for the
keyTypeAt, ExistsAt, and loadSetAt calls with this new ctx variable.
Additionally, verify whether the multi-key sharding assumption is correct since
proxyToLeader only examines cmd.Args[1] while pfcount iterates over all keys in
cmd.Args[1:]; confirm whether all keys in a single command are guaranteed to be
co-located on the same shard or if per-key routing is required.
In `@adapter/redis_stream_cmds.go`:
- Around line 215-216: Replace context.Background() with r.handlerContext() in
the context.WithTimeout calls within both the xadd handler (at lines 215-216)
and the xtrim handler (at line 546) to ensure these Redis write commands respect
server shutdown signals. This aligns with how the xread handler properly uses
r.handlerContext() at line 1052 and completes the "propagate Redis command
contexts" work. The change ensures operations will be properly cancelled during
graceful shutdown instead of potentially outliving the server's drain window.
In `@adapter/redis_zset_cmds.go`:
- Around line 849-854: The ZREM operation removes members from the zset but only
persists the remaining entries to the legacy blob key via persistZSetEntriesTxn,
leaving the wide-column ZSetMemberKey and ZSetScoreKey entries for removed
members untouched. This causes the removed members to be reloaded on the next
read. To fix this, after calling removeZSetMembers to identify which members
were removed, explicitly delete the ZSetMemberKey and ZSetScoreKey wide-column
entries for each removed member (following the same pattern used in
persistBZPopMinResult for lines 980-998) before or in conjunction with the
persistZSetEntriesTxn call.
---
Minor comments:
In `@adapter/redis_expire_cmds.go`:
- Around line 222-223: The context creation at lines 222-223 uses
context.Background() which does not propagate shutdown signals properly. Replace
the context.WithTimeout call that uses context.Background() with a call that
uses r.handlerContext() instead, while maintaining the same timeout duration
(redisDispatchTimeout). This ensures the context respects the handler's
lifecycle and proper shutdown propagation similar to other handlers in this
file.
- Around line 94-95: The context is being created using
context.WithTimeout(context.Background(), redisDispatchTimeout) which does not
properly propagate shutdowns. Replace this context creation with
r.handlerContext() which is the correct method used by other handlers in this
file to ensure proper shutdown propagation and context lifecycle management.
- Around line 30-31: Replace context.Background() with r.handlerContext() in the
context creation at the beginning of the handler to ensure proper shutdown
propagation. This aligns with the pattern used in other handlers like setLegacy
and ensures in-flight requests are properly cancelled when the server shuts
down. The ctx variable will then derive from the handler's context instead of
being a detached background context.
- Around line 180-186: In the parseExpireTTL function, replace the fmt.Errorf
call with cockerrors.Wrap to ensure proper stack trace handling according to
coding guidelines. Import the cockerrors package from
github.com/cockroachdb/errors if not already imported, and use cockerrors.Wrap
to wrap the error instead of fmt.Errorf with the %w verb.
- Around line 46-47: The context initialization at lines 46-47 is creating a new
context from context.Background() instead of using the handler's context for
proper shutdown propagation. Replace the context.WithTimeout call to derive from
r.handlerContext() instead of context.Background(), ensuring consistency with
the setex implementation and allowing proper shutdown behavior throughout the
handler.
- Around line 3-16: The file has mixed error package imports (standard errors
and cockerrors) which violates coding guidelines. Remove the standard `errors`
import from the import block and replace the `errors.New("ERR syntax error")`
call (referenced in the content at line 169) with `cockerrors.New("ERR syntax
error")` to ensure consistent use of the cockroachdb errors package for proper
stack traces and error wrapping throughout the file.
In `@adapter/redis_keys.go`:
- Around line 205-237: The matchesAsteriskPattern function has a bug where the
suffix check using HasSuffix does not account for prefix/suffix overlap. When
checking if the key ends with the suffix portion (the last part after splitting
by asterisk), add a validation that the suffix does not overlap with the
already-consumed prefix position. Specifically, ensure that before calling
HasSuffix for the last part, verify that the position where the suffix would
need to start (len(key) - len(last)) is greater than or equal to the current
position (pos) to prevent patterns like "a*a" from incorrectly matching keys
like "a" where the prefix and suffix overlap.
In `@adapter/redis_set_cmds.go`:
- Line 577: The loadSetAt function call is using context.Background() which
ignores any timeout settings from the outer context. Replace
context.Background() with ctx in the loadSetAt call to properly propagate the
outer context's timeout and cancellation behavior through the function call.
- Around line 461-484: The sismember function uses context.Background() without
a timeout, which could cause unbounded blocking on store operations, while other
command handlers in the codebase use context.WithTimeout. Replace the ctx
initialization in the sismember function to use context.WithTimeout instead of
context.Background(), applying the same timeout pattern used by other command
handlers (referenced at lines 451 and 568). This ensures the context passed to
setMemberFastExists and sismemberSlow methods has appropriate timeout
protection.
- Around line 534-562: The smembers function uses context.Background() without a
timeout in both the r.keyTypeAt and r.loadSetAt calls, which can cause unbounded
blocking. Create a timeout-scoped context at the beginning of the smembers
function (following the pattern used in write handlers) and replace both
context.Background() calls with this timeout context to ensure consistency and
prevent unbounded blocking operations.
- Around line 30-34: The validateExactSetKind function is using
context.Background() which ignores timeout and cancellation signals from parent
contexts in retry loops. Add a context parameter to the validateExactSetKind
function signature, replace the context.Background() call with this new
parameter when calling r.keyTypeAt(), and then update all three callers of
validateExactSetKind (referenced at lines 186, 211, and 573) to pass their own
context instead of letting the function create a new one.
In `@adapter/redis_zset_cmds.go`:
- Around line 825-860: The zrem method creates a context with timeout using ctx,
cancel := context.WithTimeout(...) but then passes context.Background() instead
of ctx to the loadZSetAt call on the line that loads the ZSet value. Replace the
context.Background() argument in the loadZSetAt call with the ctx variable to
ensure proper timeout propagation and cancellation behavior throughout the
operation.
- Around line 862-911: In the zremrangebyrank function, replace the
context.Background() argument in the r.loadZSetAt call with ctx to ensure the
timeout created at the start of the function is properly propagated to all
operations. The ctx variable with redisDispatchTimeout is already defined
earlier in the function and should be used consistently throughout instead of
creating a new background context.
- Around line 920-970: In the tryBZPopMinWithMode function, the call to
r.loadZSetAt is using context.Background() instead of the ctx variable that was
created with a timeout at the beginning of the function. Replace
context.Background() with ctx in the r.loadZSetAt call to ensure proper timeout
propagation throughout the operation.
---
Nitpick comments:
In `@adapter/redis_proxy_leader.go`:
- Around line 226-245: The getOrCreateLeaderClient method caches Redis clients
indefinitely without ever closing them, leading to connection leaks when
leadership changes. Add a cleanup mechanism to close and remove stale clients
from the r.leaderClients cache when they are no longer needed, such as closing
the old client before storing a new one for the same address, or implementing a
separate method to explicitly close and evict clients from the cache when
leadership transitions occur. Ensure the cleanup respects the existing mutex
synchronization (r.leaderClientsMu) to prevent race conditions.
In `@adapter/redis_server_cmds.go`:
- Around line 490-495: In the quit method of the RedisServer type, replace the
log.Printf call with slog for structured logging to maintain consistency with
the project's logging standards. Use the appropriate slog function (such as
slog.Error) to log the error when conn.Close() fails, ensuring the error
information is preserved in the structured log output.
- Around line 571-577: In the publish method of the RedisServer type, replace
the log.Printf call used for trace logging with structured logging using slog.
Extract the trace message components (remote address from conn.RemoteAddr(),
channel from cmd.Args[1], and subscriber count) as structured key-value pairs to
pass to slog, following the same pattern used in the quit function as referenced
in the guidelines.
In `@adapter/redis_set_cmds.go`:
- Around line 46-52: In the hllExistsAt function, replace the fmt.Errorf call
with cockerrors.Wrap to maintain consistency with error wrapping practices in
the rest of the file. The cockerrors.Wrap function should be called with the
error returned from r.store.ExistsAt and a descriptive error message like
"exists hll" to provide proper error context.
In `@adapter/redis_stream_cmds.go`:
- Around line 1143-1165: The read operations in this stream command handler are
using context.Background() which provides no timeout for the keyTypeAtExpect,
loadStreamMetaAt, and loadStreamAt calls, potentially allowing reads to hang
indefinitely and making it inconsistent with how other commands like xread
derive their context. Replace all instances of context.Background() in this code
block with a context derived from r.handlerContext() to ensure these read
operations have proper timeout handling and can be cancelled during graceful
shutdown, maintaining consistency with the rest of the codebase.
In `@adapter/redis_zset_cmds.go`:
- Around line 792-823: The zrangeRead function uses context.Background() for
both the keyTypeAtExpect and loadZSetAt operations, which lacks timeout
protection unlike write handlers. Create a timeout context at the beginning of
the zrangeRead function (similar to how write handlers handle context creation)
and pass that timeout context to both the keyTypeAtExpect call and the
loadZSetAt call instead of context.Background() to ensure operations cannot hang
indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4205ad0d-e44c-488c-ac90-aaa093ddf6d2
📒 Files selected for processing (13)
adapter/redis.goadapter/redis_compat_commands.goadapter/redis_expire_cmds.goadapter/redis_hash_cmds.goadapter/redis_keys.goadapter/redis_lists.goadapter/redis_proxy_leader.goadapter/redis_server_cmds.goadapter/redis_set_cmds.goadapter/redis_stream_cmds.goadapter/redis_strings.goadapter/redis_txn.goadapter/redis_zset_cmds.go
Owner
Author
|
@codex review Current-head review sweep complete. The Major inline findings are already addressed in commit 4e8854b:
No additional code change was needed in this tick. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
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.
Summary
Behavior-preserving refactor that splits the two largest Redis adapter files into cohesive, same-package (
adapter) files. This is pure code movement: types/functions/methods/consts were relocated verbatim (comments moved with their code). No signatures, logic, names, or semantics changed.adapter/redis.go: 4,483 -> 935 lines (core entry kept: server struct, options,NewRedisServer, lifecycle, dispatch infra, conn state, metrics/error helpers, ping, validateCmd, pub/sub fan-out).adapter/redis_compat_commands.go: 5,434 -> 81 lines (now holds only the shared top-levelconstblock).File map (region -> new file, with line counts)
Moved out of
redis.go:redis_strings.goredis_keys.gotxnContext+ all txn methods, MULTI/DISCARD/EXEC,runTransaction,runTransactionWithDedup,dispatchExecReuse,firstExecAttempt,txnStartTS,txnApplyHandlersredis_txn.goredis_proxy_leader.golistPushCore,listPushCoreWithDedup,dispatchListPushReuse, pop/range/trim)redis_lists.goMoved out of
redis_compat_commands.go:redis_server_cmds.goredis_expire_cmds.goredis_set_cmds.goredis_hash_cmds.goredis_zset_cmds.goredis_stream_cmds.goredis_lists.go(shared dest)buildRouteMap(the dispatch table) was already inredis_command_specs.goand was left untouched.No behavior change
Pure move. Verified two independent ways:
go doc -all ./adapteridentical — captured the exported API surface against the worktree base (viagit stashof the source files) and after the split;diffreports no differences.package/importblocks from all 13 files and comparing the multiset of non-blank code lines against the two originals: 9,144 lines on both sides, 0 missing, 0 extra. The +147 net line delta in the raw diff is entirely the addedpackage adapter+import (...)boilerplate in the 11 new files.Imports were recomputed per file and the source-file alias conventions preserved exactly:
redis.go-derived files keep"github.com/cockroachdb/errors"unaliased;redis_compat_commands.go-derived files keep stdlib"errors"pluscockerrors "github.com/cockroachdb/errors", andredis_stream_cmds.gokeepsgoogle.golang.org/grpc/{codes,status}.Jepsen-guarded blocks moved intact
The recently-merged one-phase-dedup default-flip config options (#943) —
WithOnePhaseTxnDedup/WithStandaloneSetDedup— were left in place inredis.gocore. The txn/dedup machinery (txnContext+ methods,runTransaction,runTransactionWithDedup,dispatchExecReuse,firstExecAttempt,txnStartTS,txnApplyHandlers) was moved as one intact contiguous block intoredis_txn.go; the list-dedup path (listPushCoreWithDedup/dispatchListPushReuse/listPushCore) was moved intact intoredis_lists.go. No internals of these blocks were reorganized.Verification evidence
Five-lens self-review (pure move)
go docchecks prove no statement was dropped or altered. No new error-handling paths.go test -raceon the full Redis suite passes.Next()path touched.go docparity confirms the public contract is unchanged.*_test.gofiles were not split and continue to exercise the moved code (fullTestRedis|Redisrace suite green).Summary by CodeRabbit