Reduce Redis proxy replay backpressure#1146
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughHLCリース更新を非同期化し、提案タイムアウトと物理窓を更新しました。BZPOPMINの候補探索とscan計画を再構成し、Redisプロキシの接続上限・blocking replay並列度・関連文書を更新しました。 ChangesHLCリース更新
BZPOPMIN候補選択
Redisプロキシ容量設定
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RunHLCLeaseRenewal
participant renewHLCLeaseAsync
participant ProposeHLCLease
RunHLCLeaseRenewal->>renewHLCLeaseAsync: 更新提案を非同期投入
renewHLCLeaseAsync->>ProposeHLCLease: タイムアウト付きで提案
ProposeHLCLease-->>renewHLCLeaseAsync: 提案結果
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (eaf5701):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (eaf5701):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
adapter/redis_peer_limiter_test.go (1)
13-20: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftproxy poolとの一致を実際に検証してください。
このテストは
defaultRedisPerPeerConnectionCapとリテラル64の一致しか確認しておらず、DefaultElasticKVBackendOptions().PoolSizeとの不一致は検出できません。共有定数を単一ソースにするか、依存方向を壊さない統合テストで両方の既定値を比較してください。🤖 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_peer_limiter_test.go` around lines 13 - 20, Update TestRedisPeerLimiterDefaultMatchesProxyPool to compare server.peerLimiter.limit with DefaultElasticKVBackendOptions().PoolSize, removing the literal 64 assertion. Ensure the test verifies both defaults remain equal without introducing a dependency that violates package direction; use a shared constant instead if that is the established design.adapter/redis_zset_cmds.go (1)
1170-1170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winマジックナンバー
2に//nolint:mndを付与せず、名前付き定数化を検討してください。コーディングガイドラインでは
//nolintの追加は極力避け、リファクタリングを優先するよう定められています。2をbzpopminScoreScanLimitのような定数として切り出せば、.golangci.yamlのmndlinter を無効化せずに済みます。As per coding guidelines, "Avoid using
//nolintdirectives; refactor instead in Go code".♻️ 提案する修正
+// bzpopminScoreScanLimit fetches the first score-index entry plus a +// last-entry sentinel so isLast can be determined without a full scan. +const bzpopminScoreScanLimit = 2 + func (r *RedisServer) bzpopminWideScoreCandidateAt(ctx context.Context, key []byte, readTS uint64) (*bzpopminCandidate, error) { scorePrefix := store.ZSetScoreScanPrefix(key) scoreEnd := store.PrefixScanEnd(scorePrefix) - scoreKVs, err := r.store.ScanAt(ctx, scorePrefix, scoreEnd, 2, readTS) //nolint:mnd // first entry plus last-entry sentinel + scoreKVs, err := r.store.ScanAt(ctx, scorePrefix, scoreEnd, bzpopminScoreScanLimit, 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` at line 1170, Replace the literal scan limit 2 and its //nolint:mnd directive in the ScanAt call with a clearly named constant such as bzpopminScoreScanLimit, preserving the existing behavior of scanning the first entry plus the last-entry sentinel.Source: Coding guidelines
adapter/redis_bzpopmin_wake_test.go (1)
30-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win候補選択の他の分岐(
isLast=true、member-only/legacyフォールバック)がテストされていません。現在のテストは wide-score-index かつ
isLast=falseの1ケースのみです。特にisLast=trueはdeleteLogicalKeyElemsによる全体削除に分岐する重要なパスなので、table-driven形式で以下も追加することを推奨します。
- スコアエントリが1件のみ(
isLast=true)- スコアインデックスなし・member-onlyのみ存在
- スコア/member両方なし・legacy blobのみ存在
As per coding guidelines, "Unit tests should be co-located with packages in
*_test.gofiles; prefer table-driven test cases."🤖 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_bzpopmin_wake_test.go` around lines 30 - 54, Expand TestRedis_BZPopMinCandidateUsesScoreIndex into table-driven cases covering the existing wide-score-index path, a single score entry that produces isLast=true, member-only fallback without a score index, and legacy-blob fallback when neither score nor member entries exist. For each case, construct the appropriate store state and assert the candidate selection and deletion-path flags, especially isLast=true for the single-entry case.
🤖 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 `@docs/architecture_overview.md`:
- Line 173: Update the architecture diagram’s “every hlcRenewalInterval” label
to explicitly state the operational 2-second renewal interval while retaining
the under-30-second physical-window constraint, using wording such as “=2s;
<30s”.
In `@docs/design/2026_06_12_proposed_scaling_roadmap.md`:
- Around line 88-89: HLC更新主体の説明を現行実装に合わせ、`ProposeHLCLease` と
`hlcPhysicalWindowMs`／`hlcRenewalInterval` の更新をdefault-group leader限定ではなく各shard
group単位のrenewalとして記述してください。続くWAN依存モデルおよびper-group
renewalの説明も同じ契約に統一し、更新間隔の値は維持してください。
In `@kv/coordinator.go`:
- Around line 44-46: hlcRenewalInterval
の値だけを根拠にHLC窓の失効が防止されるような定義・コメントにせず、提案処理の最大時間、スケジューラ停止、連続更新失敗を含む安全余裕を明示的に反映してください。hlcRenewalInterval
と hlcPhysicalWindowMs の関係を、その余裕を満たすことまで検証できる形に更新し、将来の値変更で誤った安全性判断が生じないようにしてください。
In `@README.md`:
- Line 17:
README.md:17のHLC説明を条件付き表現に変更し、新リーダーが重複タイムスタンプを発行しないことを実装済み保証として断定しない。docs/design/2026_06_11_implemented_leader_balance_scheduler.md:192のリーダー移動時の安全性説明にも、論理カウンタ引き継ぎとceiling
expiry時のfail-closed
fencingが実装されていることを前提条件として明記する。両文書の記述を、これらの安全機構が実装されるまで保証ではなく必要条件として整合させる。
---
Nitpick comments:
In `@adapter/redis_bzpopmin_wake_test.go`:
- Around line 30-54: Expand TestRedis_BZPopMinCandidateUsesScoreIndex into
table-driven cases covering the existing wide-score-index path, a single score
entry that produces isLast=true, member-only fallback without a score index, and
legacy-blob fallback when neither score nor member entries exist. For each case,
construct the appropriate store state and assert the candidate selection and
deletion-path flags, especially isLast=true for the single-entry case.
In `@adapter/redis_peer_limiter_test.go`:
- Around line 13-20: Update TestRedisPeerLimiterDefaultMatchesProxyPool to
compare server.peerLimiter.limit with DefaultElasticKVBackendOptions().PoolSize,
removing the literal 64 assertion. Ensure the test verifies both defaults remain
equal without introducing a dependency that violates package direction; use a
shared constant instead if that is the established design.
In `@adapter/redis_zset_cmds.go`:
- Line 1170: Replace the literal scan limit 2 and its //nolint:mnd directive in
the ScanAt call with a clearly named constant such as bzpopminScoreScanLimit,
preserving the existing behavior of scanning the first entry plus the last-entry
sentinel.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ee592f7-7d7c-4bda-8e99-5d8617f10bb2
📒 Files selected for processing (21)
README.mdadapter/redis_bzpopmin_wake_test.goadapter/redis_peer_limiter.goadapter/redis_peer_limiter_test.goadapter/redis_zset_cmds.gocmd/redis-proxy/main_test.godeploy/redis-proxy/docker-compose.ha.ymldocs/architecture_overview.mddocs/design/2026_04_24_implemented_workload_isolation.mddocs/design/2026_05_28_implemented_tla_safety_spec.mddocs/design/2026_06_02_implemented_idempotent_snapshot_restore.mddocs/design/2026_06_11_implemented_leader_balance_scheduler.mddocs/design/2026_06_12_proposed_scaling_roadmap.mddocs/redis-proxy-deployment.mdkv/coordinator.gokv/lease_read_test.gokv/lease_warmup_test.gokv/sharded_coordinator.goproxy/backend.goproxy/dualwrite.goproxy/proxy_test.go
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (c174055):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1740550db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| candidate, err := r.bzpopminWideScoreCandidateAt(ctx, key, readTS) | ||
| if err != nil || candidate != nil { | ||
| return candidate, err | ||
| } | ||
| candidate, err = r.bzpopminMemberOnlyCandidateAt(ctx, key, readTS) |
There was a problem hiding this comment.
When a zset still has member-only wide rows from the older layout and a later ZADD/ZINCRBY touches only some members, only those touched members get !zs|scr| score-index rows. This score-first branch then treats any partial score index as authoritative, so BZPOPMIN can pop a higher-scored indexed member while lower-scored unindexed members remain; if there is only one indexed member, isLast is set and persistBZPopMinResult deletes the whole logical key, dropping the unindexed members. Please fall back to the member scan/load path unless the score index is known to cover the full zset, or migrate all member rows before using the score index.
Useful? React with 👍 / 👎.
# Conflicts: # adapter/redis_peer_limiter.go # adapter/redis_peer_limiter_test.go # docs/redis-proxy-deployment.md # kv/lease_read_test.go # proxy/backend.go # proxy/proxy_test.go
# Conflicts: # adapter/redis_peer_limiter_test.go # docs/redis-proxy-deployment.md
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (64e97df):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e97dfc1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| return &bzpopminCandidate{ | ||
| entry: redisZSetEntry{Member: string(member), Score: score}, | ||
| isWide: true, | ||
| isLast: len(scoreKVs) == 1, |
There was a problem hiding this comment.
When a wide ZSET is in a mixed upgrade state (member rows exist but only the newly written lowest member has a score-index row), this sets isLast from the score-index scan count rather than the member-row count. bzpopminCandidateAt can then pick this lower score candidate, and persistBZPopMinResult treats it as the last entry and calls deleteLogicalKeyElems, deleting the remaining unindexed members instead of popping only the returned member.
Useful? React with 👍 / 👎.
| redisPeerLimitError = "ERR max connections per client exceeded" | ||
| unknownRedisPeer = "unknown" | ||
| redisPerPeerLimitEnv = "ELASTICKV_REDIS_PER_PEER_CONNECTIONS" | ||
| defaultRedisPerPeerConnectionCap = redislimits.DefaultElasticKVRedisConnections |
There was a problem hiding this comment.
With the new default, the adapter per-peer cap equals the proxy's default ElasticKV command pool size (64). In modes where the proxy also opens ElasticKV Pub/Sub/shadow Pub/Sub connections, those sockets are dedicated outside the go-redis command pool, so a busy proxy host can fill all 64 pooled command connections and then have the Pub/Sub connection rejected by this limiter; the previous dedicated headroom avoided that user-visible failure.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a3ed3ec):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
…onnections # Conflicts: # adapter/redis_peer_limiter_test.go # docs/redis-proxy-deployment.md # proxy/backend.go
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (c14b20c):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c14b20c3c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (9193b0b):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9193b0bbf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| return &bzpopminCandidate{ | ||
| entry: redisZSetEntry{Member: string(member), Score: score}, | ||
| isWide: true, | ||
| isLast: scoreIndexComplete && metaLen == 1, | ||
| }, scoreIndexComplete, nil |
There was a problem hiding this comment.
When BZPOPMIN runs on the Pebble-backed store and the minimum-score bucket has members whose names are prefix-related, such as a and aa, returning the first raw score-index scan row can pop the wrong member. Pebble orders encoded MVCC keys as userKey || invertedTS, so ...aa... can sort before ...a<ts>... even though Redis sorted sets break equal-score ties by member lexicographic order; the existing range fast path avoids this by falling back on duplicate scores. This path should either sort/verify all tied minimum-score rows or fall back to the member loader when the score scan shows a tie before deleting the candidate.
Useful? React with 👍 / 👎.
| // ceiling = now + window, and renews before the window expires. A new leader | ||
| // inherits the committed ceiling so it never issues timestamps that collide | ||
| // with the previous leader's window. | ||
| const hlcPhysicalWindowMs int64 = 30_000 |
There was a problem hiding this comment.
With this window set to 30s, a normal renewal makes NextFenced allocate prepare startTS values around now+30s, but txnLockExpireAt still sets the default lock expiry to wall now+defaultTxnLockTTLms, also 30s. For a two-phase/cross-shard transaction prepared soon after a renewal, the lock's ExpireAt is therefore at or only milliseconds above its own startTS, and the MVCC visibility check treats ExpireAt <= readTS as expired, so later transactions stop seeing the lock after the next renewal instead of after the intended 30s. Either keep the window comfortably below the default lock TTL or compute lock expiry from the HLC start timestamp.
Useful? React with 👍 / 👎.
| close hooks. Default cap is 64 per peer IP and is configurable via | ||
| `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` / |
There was a problem hiding this comment.
This updated status says the Redis adapter's per-peer cap defaults to 64, but the implementation now derives defaultRedisPerPeerConnectionCap as the 64-connection ElasticKV pool plus 64 dedicated headroom, i.e. 128. Operators following this doc may set ELASTICKV_REDIS_PER_PEER_CONNECTIONS to only the proxy pool size and lose the intended room for Pub/Sub/detached sockets, recreating the connection rejects this change is trying to avoid.
Useful? React with 👍 / 👎.
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (47ebe6e):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47ebe6ea70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
Evidence for current HEAD cfd495d: Addressed the latest documentation findings:
Validation:
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (cfd495d):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfd495d75b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if metaLen >= int64(maxWideScanLimit) { | ||
| return maxWideScanLimit | ||
| } | ||
| return int(metaLen) + 1 |
There was a problem hiding this comment.
For normal wide zsets with metadata, this sets the ScanAt limit to metaLen + 1 (capped at 100001), so a hot BZPOPMIN on a 50k-member sorted set still reads essentially the entire score index before popping one member. That preserves much of the O(N) replay cost this change is trying to remove; the score-index fast path should keep the initial scan bounded to the first couple of score rows and only fall back to the full member load when the bounded probe cannot be trusted (e.g. ties or incomplete-index detection).
Useful? React with 👍 / 👎.
| (iii) makes the ceiling-fence behaviour normative: a leader | ||
| partitioned from the default group's quorum for longer than | ||
| `hlcPhysicalWindowMs` (currently 3s) cannot serve any persistence | ||
| `hlcPhysicalWindowMs` (currently 20s) cannot serve any persistence |
There was a problem hiding this comment.
This updated 20s risk item still describes HLC outage as partition from the default group's quorum and says the current implementation silently keeps issuing timestamps, but the code now renews HLC leases for led shard groups and persistence allocation goes through NextFenced/RecoverHLCLease. Operators reading the implemented TLA spec will use the wrong quorum/fail-closed model when evaluating partitions, so this section should be updated to match per-group renewal and fenced timestamp allocation.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (073aee6):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Addressed the latest-head review items in 073aee6.
Validation:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/redis-proxy-deployment.md (1)
38-38: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winpeer capの説明をコード側の前提に合わせてください。
Line [38] の「
ELASTICKV_REDIS_PER_PEER_CONNECTIONS以下」は、同文書の Line [447] およびproxy/backend.goLines [76-81] の「プールより大きい上限を設定し、Pub/Sub・detached socket用の余裕を残す」という説明と矛盾します。上限とpool sizeを同値にすると専用接続分の余裕がなくなるため、headroomを明示してください。修正例
-| `-elastickv-pool-size` | `64` | ElasticKV backend connection pool size; keep this at or below `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` on the cluster | +| `-elastickv-pool-size` | `64` | ElasticKV backend connection pool size; configure `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` above this value to leave headroom for dedicated and detached sockets |🤖 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 `@docs/redis-proxy-deployment.md` at line 38, Update the -elastickv-pool-size description to match the backend connection logic: state that ELASTICKV_REDIS_PER_PEER_CONNECTIONS must be greater than the pool size, leaving headroom for Pub/Sub and detached-socket connections. Keep the existing configuration key and pool-size guidance unchanged.kv/sharded_coordinator.go (2)
2446-2456: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift遅い最初のグループで recovery 全体を直列化しないでください。
Line 2449 の提案が
rctxの期限まで遅延すると、後続の正常な target に一度も提案されません。全 target を並行起動して結果を集約し、遅延 target と即時成功 target の組合せを回帰テストしてください。🤖 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 `@kv/sharded_coordinator.go` around lines 2446 - 2456, Update recoverHLCLeaseTargets to launch proposeHLCLeaseForGroup concurrently for every target instead of processing targets serially, then wait for all attempts and combine their errors using the existing recovery error aggregation. Preserve the ceiling-expiry check and hlcRecoveryFailedError behavior, and add a regression test covering a delayed target alongside an immediately successful target.
2459-2471: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
errors.CombineErrorsはgithub.com/cockroachdb/errorsv1.14.0 に存在し、このコードではコンパイルできません。
import "github.com/pkg/errors"を追加・置換するか、この箇所でもgithub.com/cockroachdb/errorsの集約 API を一貫して使用してください。現状のままではこの関数でビルド失敗します。🤖 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 `@kv/sharded_coordinator.go` around lines 2459 - 2471, Update combineHLCRecoveryErrors and hlcRecoveryFailedError to use the imported errors package consistently with the repository’s dependency version. Replace the unsupported errors.CombineErrors calls with the supported aggregation API, or switch the import to github.com/pkg/errors if that is the established usage, while preserving the existing wrapped error messages and nil-handling behavior.Source: Coding guidelines
🧹 Nitpick comments (2)
cmd/redis-proxy/main_test.go (1)
75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win本番デフォルトの入力値を共有定数から参照してください。
Line [75] の
64は、proxy/backend.goが使用するredislimits.DefaultElasticKVRedisConnectionsと重複しています。このままだと既定値が変更されても、production defaultを検証するテストが古い64を使ったまま通過します。修正例
- elasticKVPoolSize: 64, + elasticKVPoolSize: redislimits.DefaultElasticKVRedisConnections,🤖 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 `@cmd/redis-proxy/main_test.go` around lines 75 - 78, Update the test case around elasticKVPoolSize to use the shared production default constant redislimits.DefaultElasticKVRedisConnections instead of the literal 64, ensuring the expected value stays synchronized with proxy/backend.go.kv/lease_read_test.go (1)
195-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
//nolintを値検証に置き換えてください。
kv/lease_read_test.go:201の入力は生バイト列なので、binary.BigEndian.Uint64(data[1:])をint64に変換する前にuint64(math.MaxInt64)以下であることを検証してください。こうすれば Suppress directive 不要で、無効な HLC 上限も明示的に扱えます。修正案
+ rawCeilingMs := binary.BigEndian.Uint64(data[1:]) + if rawCeilingMs > uint64(math.MaxInt64) { + t.Error("invalid HLC lease ceiling") + return + } - ceilingMs := int64(binary.BigEndian.Uint64(data[1:])) //nolint:gosec // encoded from a positive Unix ms timestamp. - clock.SetPhysicalCeiling(ceilingMs) + clock.SetPhysicalCeiling(int64(rawCeilingMs))🤖 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 `@kv/lease_read_test.go` around lines 195 - 204, In applyHLCLeaseEntryToClock, validate that the decoded uint64 value is at most uint64(math.MaxInt64) before converting it to int64. Return without updating the clock for out-of-range values, then remove the nolint directive from the conversion.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@docs/redis-proxy-deployment.md`:
- Line 38: Update the -elastickv-pool-size description to match the backend
connection logic: state that ELASTICKV_REDIS_PER_PEER_CONNECTIONS must be
greater than the pool size, leaving headroom for Pub/Sub and detached-socket
connections. Keep the existing configuration key and pool-size guidance
unchanged.
In `@kv/sharded_coordinator.go`:
- Around line 2446-2456: Update recoverHLCLeaseTargets to launch
proposeHLCLeaseForGroup concurrently for every target instead of processing
targets serially, then wait for all attempts and combine their errors using the
existing recovery error aggregation. Preserve the ceiling-expiry check and
hlcRecoveryFailedError behavior, and add a regression test covering a delayed
target alongside an immediately successful target.
- Around line 2459-2471: Update combineHLCRecoveryErrors and
hlcRecoveryFailedError to use the imported errors package consistently with the
repository’s dependency version. Replace the unsupported errors.CombineErrors
calls with the supported aggregation API, or switch the import to
github.com/pkg/errors if that is the established usage, while preserving the
existing wrapped error messages and nil-handling behavior.
---
Nitpick comments:
In `@cmd/redis-proxy/main_test.go`:
- Around line 75-78: Update the test case around elasticKVPoolSize to use the
shared production default constant redislimits.DefaultElasticKVRedisConnections
instead of the literal 64, ensuring the expected value stays synchronized with
proxy/backend.go.
In `@kv/lease_read_test.go`:
- Around line 195-204: In applyHLCLeaseEntryToClock, validate that the decoded
uint64 value is at most uint64(math.MaxInt64) before converting it to int64.
Return without updating the clock for out-of-range values, then remove the
nolint directive from the conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb5a7d1c-9464-4162-a33c-e35f78696728
📒 Files selected for processing (20)
README.mdadapter/redis_bzpopmin_wake_test.goadapter/redis_peer_limiter.goadapter/redis_peer_limiter_test.goadapter/redis_zset_cmds.gocmd/redis-proxy/main_test.godocs/architecture_overview.mddocs/design/2026_04_24_implemented_workload_isolation.mddocs/design/2026_05_28_implemented_tla_safety_spec.mddocs/design/2026_06_11_implemented_leader_balance_scheduler.mddocs/design/2026_06_12_proposed_scaling_roadmap.mddocs/redis-proxy-deployment.mdinternal/redislimits/limits.gokv/coordinator.gokv/lease_read_test.gokv/lease_warmup_test.gokv/sharded_coordinator.goproxy/backend.goproxy/dualwrite.goproxy/proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- adapter/redis_peer_limiter.go
- proxy/dualwrite.go
- adapter/redis_zset_cmds.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 073aee6110
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| return nil, false, nil | ||
| } | ||
| if candidate != nil { | ||
| scoreIndexComplete := metaExists && metaLen > 0 && int64(len(scoreKVs)) == metaLen |
There was a problem hiding this comment.
When a live wide-column zset has more than two members, scoreKVs is capped by bzpopminScoreScanLimit (2), so this completeness check is always false for metaLen > 2. That makes bzpopminCandidateAt fall through to bzpopminMemberOnlyCandidateAt, which calls loadZSetMembersAt and scans/sorts the whole zset before every pop; for the large blocking queues this change is meant to protect, the score-index path therefore still pays the old O(N) member load (plus the new score probes) instead of relieving replay backpressure.
Useful? React with 👍 / 👎.
1 participant
Summary
Root cause
Production Redis proxy metrics showed the blocking replay queue filling while the ElasticKV go-redis pool had no pending requests. CPU profiling on the ElasticKV leader showed BZPOPMIN replay spending most CPU in ZSet type/probe scans and full sorted-set loading. Under this pressure, the previous 1s HLC renewal proposal timeout could miss renewals and cause write failures.
Validation
go test ./adapter -run 'TestRedisPeerLimiter|TestRedisLeaderClient|TestRedisBlockingLeaderClient|TestRedis_BZPopMin|TestRedis_BullMQ' -count=1go test ./cmd/redis-proxy ./proxy ./kv -count=1git diff --checkOperations
Temporarily updated the running proxy on
192.168.0.64to pool 192, write concurrency 96, script concurrency 16, blocking replay concurrency 64, and blocking replay queue 8192. After the change, proxy metrics showed drop count 0, secondary write errors 0, queue depth 0, and no backend pool wait.Summary by CodeRabbit
機能改善
ドキュメント