adapter: split redis.go / redis_compat_commands.go into cohesive files (no behavior change)#956
adapter: split redis.go / redis_compat_commands.go into cohesive files (no behavior change)#956bootjp wants to merge 1 commit into
Conversation
…s (no behavior change)
|
@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.
|
Warning Review limit reached
More reviews will be available in 33 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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) |
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.
| 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) |
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.
| 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) |
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.
| 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) |
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.
| 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) |
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.
| 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) |
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.
| 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 { |
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) |
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)| 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) |
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.
| 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() |
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.
1 participant
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).