fix(rpc): derive /raft/node leadership from raft leader ID#3266
fix(rpc): derive /raft/node leadership from raft leader ID#3266
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @auricom's task in 1m 45s —— View job Code Review
SummaryThis PR correctly fixes the stale leadership state bug by deriving What's Good
IssuesSuggestion: expose
|
📝 WalkthroughWalkthroughA new public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/raft/node.go (1)
211-216: Add a doc comment for exportedLeaderID.Other exported methods on
Node(e.g.,IsLeader,HasQuorum) are documented;LeaderIDshould be as well.📝 Proposed doc
+// LeaderID returns the raft leader's server ID, or "" if unknown or the node is not initialized. func (n *Node) LeaderID() string { if n == nil || n.raft == nil { return "" } return n.leaderID() }As per coding guidelines: "Document exported types and functions in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/raft/node.go` around lines 211 - 216, Add a Go doc comment for the exported function LeaderID on type Node: place a comment immediately above func (n *Node) LeaderID() string that describes what LeaderID returns (the current cluster leader's ID as a string), its behavior when the receiver is nil or uninitialized (returns empty string), and any guarantees or semantics (e.g., may be stale until raft leadership changes or only valid when raft != nil). Use the style of the existing comments for Node.IsLeader and Node.HasQuorum for consistency.pkg/rpc/server/http_test.go (2)
71-75: Test intent would be clearer by removing the misleadingisLeader: false.Setting
isLeader: falsewhile assertingbody.IsLeader == trueis correct (the handler derives fromLeaderID), but makes the test read ambiguously — a reader might think the assertion is wrong. Either drop the field (zero value isfalseanyway, which actually reinforces the point) or add a comment noting thatIsLeader()is intentionally stale here to verifyLeaderID()takes precedence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/server/http_test.go` around lines 71 - 75, The test uses testRaftNodeSource with isLeader:false while asserting body.IsLeader == true, which is confusing; update the test by either removing the isLeader field (let it use the zero value) or add a short clarifying comment next to the testRaftNodeSource instantiation stating that IsLeader() is intentionally stale and the handler should derive leadership from LeaderID() via LeaderID(); reference testRaftNodeSource, IsLeader, LeaderID, and body.IsLeader in the comment or change so the intent is explicit.
67-147: Consider consolidating the three/raft/nodetests into a table-driven test.The three new tests are largely duplicated setup/teardown with only input and expected output differing. A single table-driven test would be more idiomatic and easier to extend (e.g., leader mismatch case:
leaderID="node-b", nodeID="node-a"→is_leader=false, which is currently not covered and is arguably the most important new behavior).♻️ Suggested structure
func TestRegisterCustomHTTPEndpoints_RaftNodeStatus(t *testing.T) { specs := map[string]struct { src testRaftNodeSource method string expStatus int expIsLeader bool expNodeID string }{ "leader id matches node id": {src: testRaftNodeSource{leaderID: "node-a", nodeID: "node-a"}, method: http.MethodGet, expStatus: http.StatusOK, expIsLeader: true, expNodeID: "node-a"}, "leader id differs from node id": {src: testRaftNodeSource{leaderID: "node-b", nodeID: "node-a"}, method: http.MethodGet, expStatus: http.StatusOK, expIsLeader: false, expNodeID: "node-a"}, "empty leader id falls back": {src: testRaftNodeSource{isLeader: false, leaderID: "", nodeID: "node-a"}, method: http.MethodGet, expStatus: http.StatusOK, expIsLeader: false, expNodeID: "node-a"}, "non-GET is rejected": {src: testRaftNodeSource{}, method: http.MethodPost, expStatus: http.StatusMethodNotAllowed}, } for name, tc := range specs { t.Run(name, func(t *testing.T) { /* ... */ }) } }Adding the "leader id differs from node id" case is especially worthwhile — it's the exact scenario the fix targets (node that stepped down but previously reported itself as leader). As per coding guidelines: "Use table-driven tests where appropriate".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/server/http_test.go` around lines 67 - 147, Consolidate the three near-duplicate tests TestRegisterCustomHTTPEndpoints_RaftNodeStatus, TestRegisterCustomHTTPEndpoints_RaftNodeStatusFallsBackWithoutLeaderID, and TestRegisterCustomHTTPEndpoints_RaftNodeStatusMethodNotAllowed into a single table-driven test that iterates over cases (using testRaftNodeSource entries) and calls RegisterCustomHTTPEndpoints for each case; include cases for leader-matches (leaderID == nodeID → is_leader true), leader-differs (leaderID != nodeID → is_leader false), empty leaderID fallback, and non-GET method expecting StatusMethodNotAllowed, and assert expected HTTP status and decoded response fields (IsLeader, NodeID) per case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/raft/node.go`:
- Around line 211-216: Add a Go doc comment for the exported function LeaderID
on type Node: place a comment immediately above func (n *Node) LeaderID() string
that describes what LeaderID returns (the current cluster leader's ID as a
string), its behavior when the receiver is nil or uninitialized (returns empty
string), and any guarantees or semantics (e.g., may be stale until raft
leadership changes or only valid when raft != nil). Use the style of the
existing comments for Node.IsLeader and Node.HasQuorum for consistency.
In `@pkg/rpc/server/http_test.go`:
- Around line 71-75: The test uses testRaftNodeSource with isLeader:false while
asserting body.IsLeader == true, which is confusing; update the test by either
removing the isLeader field (let it use the zero value) or add a short
clarifying comment next to the testRaftNodeSource instantiation stating that
IsLeader() is intentionally stale and the handler should derive leadership from
LeaderID() via LeaderID(); reference testRaftNodeSource, IsLeader, LeaderID, and
body.IsLeader in the comment or change so the intent is explicit.
- Around line 67-147: Consolidate the three near-duplicate tests
TestRegisterCustomHTTPEndpoints_RaftNodeStatus,
TestRegisterCustomHTTPEndpoints_RaftNodeStatusFallsBackWithoutLeaderID, and
TestRegisterCustomHTTPEndpoints_RaftNodeStatusMethodNotAllowed into a single
table-driven test that iterates over cases (using testRaftNodeSource entries)
and calls RegisterCustomHTTPEndpoints for each case; include cases for
leader-matches (leaderID == nodeID → is_leader true), leader-differs (leaderID
!= nodeID → is_leader false), empty leaderID fallback, and non-GET method
expecting StatusMethodNotAllowed, and assert expected HTTP status and decoded
response fields (IsLeader, NodeID) per case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02c32518-6288-421f-9aab-a1309d349144
📒 Files selected for processing (4)
pkg/raft/node.gopkg/rpc/server/http.gopkg/rpc/server/http_test.gopkg/rpc/server/server.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3266 +/- ##
==========================================
+ Coverage 62.52% 62.54% +0.02%
==========================================
Files 122 122
Lines 13012 13020 +8
==========================================
+ Hits 8136 8144 +8
+ Misses 3992 3990 -2
- Partials 884 886 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add Go doc comment to Node.LeaderID() describing return value, nil-safety, and staleness semantics, consistent with IsLeader/HasQuorum style. - Consolidate three near-duplicate TestRegisterCustomHTTPEndpoints_RaftNodeStatus* tests into a single table-driven test covering: leaderID==nodeID (is_leader true), leaderID!=nodeID (is_leader false), empty leaderID fallback, and non-GET method (405). Clarifies that is_leader is derived from LeaderID(), not the IsLeader() field on testRaftNodeSource. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Separate third-party imports from evstack/ev-node-prefixed imports per project gci custom-order config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Fixes #3265
Tested this PR on a 3 members raft cluster, with this fix, the leader detection displayed on /raft/node endpoint is instantaneous instead of taking ~ 30 sedonds.
Overview
Summary by CodeRabbit
New Features
Tests