server: return unavailable when PD is not serving - #11066
Conversation
Return gRPC Unavailable from service-discovery RPCs when the local PD server is not running. Preserve response-header errors for unrelated member-loading failures. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/M
📝 WalkthroughWalkthroughService discovery RPCs now return ChangesService discovery RPC errors
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/grpc_service_test.go (1)
138-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
%wshutdown regression case.This test only exercises
errors.WithStack, so it will not catch a standardfmt.Errorf("...: %w", ...)wrapper being misclassified. Add an assertion for the wrapped error returning a nil response andcodes.Unavailable. (pkg.go.dev)Based on coding guidelines,
%wwrapping is supported and sentinel checks should useerrors.Is/As.Proposed test addition
+ wrapped := fmt.Errorf("server is shutting down: %w", errs.ErrServerNotStarted.FastGenByArgs()) + response, err = getMembersErrorResult(wrapped) + require.Nil(t, response) + require.Equal(t, codes.Unavailable, status.Code(err))🤖 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 `@server/grpc_service_test.go` around lines 138 - 149, Add a standard fmt.Errorf %w-wrapped ErrServerNotStarted case to TestGetMembersErrorResult, asserting getMembersErrorResult returns a nil response and codes.Unavailable. Ensure the implementation’s shutdown sentinel detection uses errors.Is/As so both standard wrapping and errors.WithStack remain correctly classified.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.
Inline comments:
In `@server/grpc_service.go`:
- Around line 468-476: The getMembersErrorResult helper currently misses
standard %w-wrapped ErrServerNotStarted values; update its classification check
to use stdlib errors.Is or equivalent traversal of both Cause and Unwrap while
preserving the existing direct and stack-wrapped behavior. In
server/grpc_service_test.go lines 138-149, add coverage for a %w-wrapped
ErrServerNotStarted case and verify it returns the unavailable error rather than
a response-level failure.
---
Nitpick comments:
In `@server/grpc_service_test.go`:
- Around line 138-149: Add a standard fmt.Errorf %w-wrapped ErrServerNotStarted
case to TestGetMembersErrorResult, asserting getMembersErrorResult returns a nil
response and codes.Unavailable. Ensure the implementation’s shutdown sentinel
detection uses errors.Is/As so both standard wrapping and errors.WithStack
remain correctly classified.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c371440-dab3-464d-b4e0-7c0364510224
📒 Files selected for processing (2)
server/grpc_service.goserver/grpc_service_test.go
| func getMembersErrorResult(err error) (*pdpb.GetMembersResponse, error) { | ||
| if errors.ErrorEqual(err, errs.ErrServerNotStarted.FastGenByArgs()) { | ||
| return nil, errs.ErrNotStarted | ||
| } | ||
| // Keep other server-side failures in the response header so clients don't | ||
| // mistake them for service or transport availability failures. | ||
| return &pdpb.GetMembersResponse{ | ||
| Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, err.Error()), | ||
| }, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve ErrServerNotStarted through standard error wrapping.
The helper can misclassify %w-wrapped unavailable errors as response-level failures, while stack wrappers are correctly detected. Use stdlib errors.Is (or a check that follows both Cause() and Unwrap()), and add a %w-wrapped ErrServerNotStarted test case.
📍 Affects 2 files
server/grpc_service.go#L468-L476(this comment)server/grpc_service_test.go#L138-L149
🤖 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 `@server/grpc_service.go` around lines 468 - 476, The getMembersErrorResult
helper currently misses standard %w-wrapped ErrServerNotStarted values; update
its classification check to use stdlib errors.Is or equivalent traversal of both
Cause and Unwrap while preserving the existing direct and stack-wrapped
behavior. In server/grpc_service_test.go lines 138-149, add coverage for a
%w-wrapped ErrServerNotStarted case and verify it returns the unavailable error
rather than a response-level failure.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11066 +/- ##
==========================================
+ Coverage 79.23% 79.28% +0.04%
==========================================
Files 540 541 +1
Lines 76010 76356 +346
==========================================
+ Hits 60226 60537 +311
- Misses 11533 11562 +29
- Partials 4251 4257 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/L
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
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 (1)
server/grpc_service.go (1)
429-440: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck server state before rate limiting.
A closed server can currently return
ErrGRPCRateLimitExceededbefore reachingErrNotStarted, violating the discovery RPC contract and masking the requiredUnavailablestatus. Move theIsClosed()check beforerateLimitCheck().Proposed fix
func (s *GrpcServer) GetMembers(context.Context, *pdpb.GetMembersRequest) (*pdpb.GetMembersResponse, error) { + if s.IsClosed() { + return nil, errs.ErrNotStarted + } done, err := s.rateLimitCheck() if err != nil { return nil, err } if done != nil { defer done() } - if s.IsClosed() { - return nil, errs.ErrNotStarted - }🤖 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 `@server/grpc_service.go` around lines 429 - 440, Move the IsClosed() check in the affected RPC handler before calling rateLimitCheck(), returning errs.ErrNotStarted immediately for closed servers. Preserve the existing rate-limit error handling and defer done() only after the server-state check passes.
🤖 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 `@server/grpc_service.go`:
- Around line 429-440: Move the IsClosed() check in the affected RPC handler
before calling rateLimitCheck(), returning errs.ErrNotStarted immediately for
closed servers. Preserve the existing rate-limit error handling and defer done()
only after the server-state check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61e3b14e-215e-4886-95b9-47fc3a084464
📒 Files selected for processing (2)
server/grpc_service.goserver/grpc_service_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/grpc_service_test.go
|
/retest |
1 similar comment
|
/retest |
1 participant
What problem does this PR solve?
Issue Number: ref #11020
GetMembersandGetClusterInforeturned a successful gRPC response with anErrorType_UNKNOWNheader when the local PD server was not running. During agraceful shutdown, this made a transient service-availability condition look
like an unclassified application error while the gRPC connection was still
READY.What is changed and how does it work?
Return the existing gRPC
Unavailableerror for these service-discovery RPCswhen the local PD server is not running. Also convert
ErrServerNotStartedreturned when member loading races with shutdown to the same gRPC status.
Other member-loading failures remain response-header errors, so unrelated
server-side failures are not reclassified as availability failures.
GetMemberscontinues to apply rate limiting before checking the running state; admitted
requests report
Unavailableduring shutdown.This does not change kvproto. The affected RPCs are read-only and clients can
safely retry them against another PD endpoint.
Check List
Tests
make gotest GOTEST_ARGS="./server -run Test\(ServiceDiscoveryRPCsReturnUnavailableWhenServerIsNotRunning\|GetMembersErrorResult\) -count=1"make gotest GOTEST_ARGS="./server -count=1".tools/bin/golangci-lint run ./server --allow-parallel-runnersmake pd-server-basic10.2.8.101, an identical shutdown probe against basefad3ff991802633a8de432ad1aa9e2ef2940f824observedresponse UNKNOWN / READYbefore the transport closed.rpc Unavailable / READY, thenrpc Unavailable / TRANSIENT_FAILURE, with no response-headerUNKNOWN.Release note
Summary by CodeRabbit
FindGroupByKeyspaceID,GetMinTS) now immediately return “Unavailable” when the service is closed.