Adjust raft tick intervals for stability - #529
Conversation
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the default Raft tick values in internal/raftengine/etcd/engine.go, increasing defaultHeartbeatTick to 10 and defaultElectionTick to 100 to achieve a 100ms heartbeat and 1s election timeout respectively. A suggestion was made to add inline comments explaining the relationship between these ticks and the tick interval to improve code maintainability.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| defaultTickInterval = 10 * time.Millisecond | ||
| defaultHeartbeatTick = 1 | ||
| defaultElectionTick = 10 | ||
| defaultHeartbeatTick = 10 // 100ms at 10ms interval |
There was a problem hiding this comment.
🚫 [golangci]reported by reviewdog 🐶
File is not properly formatted (gci)
Uh oh!
There was an error while loading. Please reload this page.
- Restore defaultElectionTick to 100 (1s / 10x heartbeat ratio). With the abort detection added in waitForLeadershipTransfer, the engine no longer needs a shorter electionTimeout to fail the transfer fast; surfacing errLeadershipTransferAborted is immediate once raft clears leadTransferee. Keeping the etcd/raft recommended 10x ratio preserves the stability intent of #529. - In checkLeadershipTransfer, match LeadTransferee against the specific target.NodeID rather than accepting any non-zero value, so an unrelated concurrent transfer does not confuse the abort detection.
…se memory at 2 GiB/peer Gemini reviewer flagged that 1024 inflight x 4 MiB per message is a 4 GiB per-peer worst-case buffered footprint. With N peers and bursty replication traffic, the aggregate can plausibly reach OOM territory on general-purpose deployments where the operator has not tuned the knob to the workload. In practice TCP backpressure on a typical LAN will cap the actually buffered bytes well below the product, but the reviewer's framing is reasonable: a compiled-in default should be conservative. Take option B from the gemini response matrix: keep MaxSizePerMsg at 4 MiB (the MsgApp-batching win that motivates this PR's narrative on small-entry KV workloads) and drop MaxInflightMsgs back to 512. This halves the worst-case product to 2 GiB per peer while preserving the batching improvement. 512 is still a 2x safety margin over the pre-#529 default of 256 that we observed filling up on followers under the pebble-seek-storm incident (PRs #560/#562/#563/#565 have since removed most of that CPU contention, so the deeper pipelining of 1024 was motivated more by belt-and-suspenders than measured need). Operators who need deeper pipelines (wide-bandwidth LAN, plenty of RAM) can still raise the knob via ELASTICKV_RAFT_MAX_INFLIGHT_MSGS without a rebuild; operators who need a tighter memory budget can lower MaxSizePerMsg via ELASTICKV_RAFT_MAX_SIZE_PER_MSG. Update the in-code rationale comment, the caller-side constant in main.go (so the engine default is actually exercised when no env override is set), and the TestNormalizeLimitConfig_DefaultsWhenUnset pin from 1024 -> 512. Drop the stale "(1024)" parenthetical in the TestOpen_InboundChannelsHonourMaxInflightEnv docblock.
No description provided.