Skip to content

Reduce Redis proxy drops and Raft snapshot load#1153

Open
bootjp wants to merge 2 commits into
mainfrom
feature/redis-proxy-conn-delta
Open

Reduce Redis proxy drops and Raft snapshot load#1153
bootjp wants to merge 2 commits into
mainfrom
feature/redis-proxy-conn-delta

Conversation

@bootjp

@bootjp bootjp commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • raise Redis proxy / ElasticKV Redis connection defaults and HA deployment docs
  • skip no-op blocking-command replays when the primary returned nil, while preserving positive BZPOP replay via ZREM
  • inline-compact hot list metadata deltas before reads hit the scan cap
  • avoid full FSM snapshot payload CRC during prewrite cleanup on the snapshot receive path

Tests

  • go test ./proxy ./cmd/redis-proxy -count=1
  • go test ./adapter -run 'Test(DeltaCompactor|ListInlineMetaCompaction|ZSetInlineMetaCompaction|RedisPeerLimiter|RedisLua_RPopLPush|Redis_LuaRPopLPush|Lua_)' -count=1\n- go test ./internal/raftengine/etcd -run 'TestPrepareFSMSnapshotWrite|TestWriteFSMSnapshotFileRoundTrip|TestOpenAndRestore|TestVerifyFSM' -count=1\n- golangci-lint --config=.golangci.yaml run --fix\n\nAuthor: bootjp

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@bootjp, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8fd02fa6-67c5-4611-824d-70d8a5de06f7

📥 Commits

Reviewing files that changed from the base of the PR and between 5d4c5ca and a30bf25.

📒 Files selected for processing (15)
  • adapter/redis_delta_compactor.go
  • adapter/redis_delta_compactor_test.go
  • adapter/redis_lua_context.go
  • adapter/redis_peer_limiter.go
  • adapter/redis_peer_limiter_test.go
  • deploy/redis-proxy/docker-compose.ha.yml
  • docs/redis-proxy-deployment.md
  • internal/raftengine/etcd/fsm_snapshot_file.go
  • kv/lease_warmup_test.go
  • kv/sharded_coordinator.go
  • kv/tso.go
  • kv/tso_test.go
  • proxy/backend.go
  • proxy/dualwrite.go
  • proxy/proxy_test.go

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

TLA+ spec divergence review (auto-triggered)

This PR touches files that the TLA+ safety spec has an anchor on (per
docs/design/2026_05_28_implemented_tla_safety_spec.md §3),
so an AI review is requested below to verify the implementation has not drifted
from the model.

Anchored files changed in this PR head (a30bf25):

  • kv/sharded_coordinator.go

What to check, by subsystem:

  • kv/hlc*.goNext() must respect the HLC-4 preconditions (i)/(ii)/(iii) from the design doc: bounded skew, logical-counter handoff on leader change (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
  • kv/coordinator.go, kv/sharded_coordinator.goRunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). Any change to renewal cadence, group selection, or fail-closed behaviour is in scope.
  • kv/transaction.go, kv/lock_resolver.go — OCC commit-ts assignment, lock-map encoding (key, lock_ts) -> start_ts, and the LockResolver action OCC-3 depends on. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
  • kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
  • store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
  • distribution/** — route catalog versioning, SplitRange atomicity, and CatalogWatcher async fan-out (M4 scope).

If the change is correct but requires a spec update, edit tla/hlc/HLC.tla (or the corresponding M2..M5 module once landed) and the design doc in the same PR. The tla-check workflow runs the TLC model check on the same paths.


@claude review please verify TLA+ spec divergence per the checklist above.

@codex review please verify TLA+ spec divergence per the checklist above.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 657681946a

ℹ️ 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".

// Prewrite cleanup runs on the snapshot receive path. Avoid a full payload
// CRC here; startup and actual restore still do full verification before
// trusting the snapshot bytes.
return fsmSnapshotFooterMatchesToken(fsmSnapPath(fsmSnapDir, index), tok.CRC32C) == nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Don't mark footer-only snapshots restorable

When prewrite cleanup sees multiple snapshot pairs, keepRestorablePrewriteSnapshots/purgeUnretainedPrewriteSnapshots keep only the newest candidate considered restorable and may delete older pairs. With this footer-only check, a .fsm whose payload is corrupt but whose 4-byte footer still matches the token is treated as restorable; cleanup can then keep that corrupt newest pair and purge the previous valid snapshot, leaving startup/restore to fail full CRC verification with no fallback. If the receive path must avoid a full CRC, avoid using the footer-only result as the retention signal that permits deleting older verified snapshots.

Useful? React with 👍 / 👎.

Comment thread proxy/backend.go
const (
defaultPoolSize = 128
defaultElasticKVPoolSize = 4
defaultElasticKVPoolSize = 64

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Leave headroom under the server connection cap

The ElasticKV backend pool now defaults to the same value as the server's default per-peer cap (defaultRedisPerPeerConnectionCap is also 64). In deployments where this pool is saturated, any Pub/Sub session against ElasticKV calls RedisBackend.NewPubSub, which opens a dedicated connection outside the pool, so the 65th connection from the same proxy host is rejected with ERR max connections per client exceeded. Keep the default pool below the cap or raise the server cap alongside it so subscribing/shadow Pub/Sub clients do not reintroduce connection drops under load.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant