Reduce Redis blocking replay pressure - #1140
Conversation
📝 WalkthroughWalkthrough
Changesブロッキング操作のリプレイ
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@codex review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8603f83157
ℹ️ 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".
|
|
||
| func secondaryBlockingReplay(cmd string, resp any) (string, []any, bool) { | ||
| switch strings.ToUpper(cmd) { | ||
| case "BZPOPMIN", "BZPOPMAX": |
There was a problem hiding this comment.
Replay all mutating blocking commands
ClassifyCommand still routes mutating blocking commands such as BLPOP/BRPOP/BRPOPLPUSH/BLMOVE/BLMPOP and XREADGROUP ... BLOCK into Blocking, but this helper now returns ok only for BZPOPMIN/BZPOPMAX. Redis documents BLPOP as removing the returned element (https://redis.io/docs/latest/commands/blpop/) and XREADGROUP as @write with delivered messages tracked in the PEL (https://redis.io/docs/latest/commands/xreadgroup/), so in dual-write modes a successful primary call for these commands now queues no secondary mutation and can leave consumed jobs or consumer-group state behind on the secondary after failover/migration. Please either keep replaying these write-like blocking commands or translate them deterministically as well.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return "", nil, false | ||
| } | ||
| return "ZREM", []any{"ZREM", key, member}, true |
There was a problem hiding this comment.
Preserve ordering for BZPOP removals
When a producer ZADD is followed by a consumer BZPOPMIN on the primary before the secondary has applied the ZADD, this replayed ZREM can run first and remove 0 rows; dispatchAsyncQueue starts queued writes in separate goroutines (proxy/async_queue.go:74-76), so an earlier enqueued ZADD is not guaranteed to reach the secondary before this new removal. If the ZADD then applies later, the item that was already popped on the primary is resurrected on the secondary after failover/migration. The previous bounded blocking replay would wait for the secondary item instead of no-oping, so this deterministic replay needs ordering with prior writes or a retry/conditional path when the removal observes no member.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
8603f83 to
da952d6
Compare
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proxy/proxy_test.go (1)
639-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BZPOPMAXも同じ再生契約で検証してください。このテストは
BZPOPMINのみを検証しています。BZPOPMAXについても、返却された key/member がZREMに変換されるケースを追加してください。両者はセットアップと期待値が同じなので、表駆動テストにまとめると契約を明確に維持できます。As per coding guidelines, “Unit tests must 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 `@proxy/proxy_test.go` around lines 639 - 665, The test currently covers only BZPOPMIN replay behavior; extend it to verify the same contract for BZPOPMAX. Refactor TestDualWriter_Blocking_ReplaysBZPopMinAsZRem into a table-driven test covering both commands, using the returned key/member to assert the secondary call is ZREM with the expected arguments.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.
Nitpick comments:
In `@proxy/proxy_test.go`:
- Around line 639-665: The test currently covers only BZPOPMIN replay behavior;
extend it to verify the same contract for BZPOPMAX. Refactor
TestDualWriter_Blocking_ReplaysBZPopMinAsZRem into a table-driven test covering
both commands, using the returned key/member to assert the secondary call is
ZREM with the expected arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b272418a-0d33-4c28-b027-dc9e9a5681ec
📒 Files selected for processing (3)
proxy/blocking.goproxy/dualwrite.goproxy/proxy_test.go
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
Author: bootjp
Summary
Risk
Tests