client: reduce membership refreshes during transport outages - #11037
client: reduce membership refreshes during transport outages#11037rleungx wants to merge 7 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
|
[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/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMember refresh gains transport-failure classification, per-URL episode tracking, degraded-mode connection inspection, scheduled-refresh suppression, recovery logging, and service-client integration. Tests cover error contracts, state transitions, concurrency, logging, allocation behavior, and benchmarks. ChangesMember refresh failure control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
b630e07 to
989aee5
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
client/servicediscovery/service_discovery_test.go (1)
474-477: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
require.NoErroris called from the bufconn server goroutines in both new tests.requirecallst.FailNow, which must run on the test goroutine, and a late failure after cleanup panics with "Log in goroutine after test has completed".
client/servicediscovery/service_discovery_test.go#L474-L477: replacerequire.NoError(t, server.Serve(listener))with at.Errorf(ignoringgrpc.ErrServerStopped).client/servicediscovery/service_discovery_test.go#L555-L558: apply the same replacement inTestUpdateMemberClearsUnobservedFailureEpisodesAfterRecovery.🤖 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 `@client/servicediscovery/service_discovery_test.go` around lines 474 - 477, Replace the goroutine-side require.NoError calls around server.Serve in client/servicediscovery/service_discovery_test.go:474-477 and client/servicediscovery/service_discovery_test.go:555-558 with non-fatal t.Errorf handling, ignoring the expected grpc.ErrServerStopped result so failures are reported safely from the test goroutines.
🧹 Nitpick comments (2)
client/servicediscovery/service_discovery.go (1)
566-631: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the degraded-branch ticker read nil-safe.
case <-connectionStateTicker.Cpanics if the invariant "degraded ⇒ ticker started" is ever broken by a future edit (e.g. a newenterDegradedcall site withoutstartConnectionStateTicker). Selecting on a channel variable is free and self-protecting, since a nil channel blocks forever.♻️ Suggested hardening
var connectionStateTicker *time.Ticker + var connectionStateCh <-chan time.Time stopConnectionStateTicker := func() { if connectionStateTicker != nil { connectionStateTicker.Stop() connectionStateTicker = nil + connectionStateCh = nil } } defer stopConnectionStateTicker() startConnectionStateTicker := func() { if connectionStateTicker == nil { connectionStateTicker = time.NewTicker(memberConnectionStateCheckInterval) + connectionStateCh = connectionStateTicker.C } } @@ - case <-connectionStateTicker.C: + case <-connectionStateCh:🤖 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 `@client/servicediscovery/service_discovery.go` around lines 566 - 631, Make the degraded-branch ticker select nil-safe by reading the ticker channel through a separate channel variable that is nil when connectionStateTicker is nil, rather than dereferencing connectionStateTicker.C directly in the select. Preserve the existing ticker behavior when startConnectionStateTicker has initialized it.client/servicediscovery/member_refresh_controller_test.go (1)
104-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBranching on the test-case name is brittle. Prefer an explicit field (e.g.
currentURLs []string) in the table so renaming a case can't silently change coverage.♻️ Suggested change
testCases := []struct { name string connections []memberConnection + currentURLs []string wait bool }{ @@ { name: "url replacement restores normal behavior", connections: observedMemberConnections(connectivity.TransientFailure, connectivity.TransientFailure), + currentURLs: []string{"url-0", "replacement-url"}, }, @@ currentURLs := initialURLs - if testCase.name == "url replacement restores normal behavior" { - currentURLs = []string{"url-0", "replacement-url"} + if testCase.currentURLs != nil { + currentURLs = testCase.currentURLs }🤖 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 `@client/servicediscovery/member_refresh_controller_test.go` around lines 104 - 149, Replace the name-based condition in the test loop with an explicit currentURLs field on each test case. Set that field only for the URL replacement scenario, use it when calling shouldWait, and otherwise default to initialURLs so renaming test cases cannot alter coverage.
🤖 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 `@client/servicediscovery/service_discovery_test.go`:
- Around line 474-477: Replace the goroutine-side require.NoError calls around
server.Serve in client/servicediscovery/service_discovery_test.go:474-477 and
client/servicediscovery/service_discovery_test.go:555-558 with non-fatal
t.Errorf handling, ignoring the expected grpc.ErrServerStopped result so
failures are reported safely from the test goroutines.
---
Nitpick comments:
In `@client/servicediscovery/member_refresh_controller_test.go`:
- Around line 104-149: Replace the name-based condition in the test loop with an
explicit currentURLs field on each test case. Set that field only for the URL
replacement scenario, use it when calling shouldWait, and otherwise default to
initialURLs so renaming test cases cannot alter coverage.
In `@client/servicediscovery/service_discovery.go`:
- Around line 566-631: Make the degraded-branch ticker select nil-safe by
reading the ticker channel through a separate channel variable that is nil when
connectionStateTicker is nil, rather than dereferencing connectionStateTicker.C
directly in the select. Preserve the existing ticker behavior when
startConnectionStateTicker has initialized it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc343168-a22b-4296-a48e-5ac2129e4e59
📒 Files selected for processing (4)
client/servicediscovery/member_refresh_controller.goclient/servicediscovery/member_refresh_controller_test.goclient/servicediscovery/service_discovery.goclient/servicediscovery/service_discovery_test.go
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11037 +/- ##
==========================================
+ Coverage 79.25% 79.29% +0.03%
==========================================
Files 541 541
Lines 76037 76227 +190
==========================================
+ Hits 60262 60443 +181
- Misses 11534 11535 +1
- Partials 4241 4249 +8
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>
rleungx
added
the
release-note
ti-chi-bot
Bot
removed
do-not-merge/release-note-label-needed
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
1 participant
What problem does this PR solve?
Issue Number: ref #11020
During a sustained PD transport outage, asynchronous membership-check triggers can keep
checkMembershipChpopulated. Each trigger starts another one-secondGetMembersretry batch, causing continuous RPC bursts and repetitive per-URL logs.What is changed and how does it work?
IDLE,CONNECTING, orTRANSIENT_FAILURE.GetMembers;Connect()forIDLEconnections;READY, is missing, is shut down, or the member URL set changes;CheckMemberChangedcalls immediate and unsuppressed.For this PR, an availability failure means an explicit gRPC dial failure or an RPC failure classified as
UnavailableorDeadlineExceeded(including a local context deadline). An availability failure alone neither identifies a transport outage nor enables request suppression; every corresponding connection must independently satisfy the connectivity gate above. Response-header errors, cluster-ID mismatches, and other application-level or uncertain failures retain the existing retry behavior.Scope relative to #11020
The issue initially proposed sampling only the per-URL log while leaving retry
frequency unchanged. This PR intentionally extends that proposal: it suppresses
only asynchronous membership refresh batches after both a complete retry batch
and every current gRPC connection independently confirms a transport outage.
Synchronous refreshes remain immediate, local connection-state changes resume the
existing retry batch, and the existing one-minute tick performs a real safety
sweep. This stricter gating is the compatibility boundary for request suppression
in this PR.
Compatibility and correctness
updateMemberremains a compatibility wrapper and returns the existing error unchanged.End-to-end validation
v8.5.5instances in TiUP Playground; TSO every 100 ms;asynchronous member check every 20 ms; all PD processes stopped gracefully.
39b6220; behavioral PR head3b3e570(222a48cisnaming-only).
[pd] failed to update member[pd] cannot update member from this urlsweep reported 28 failed attempts and 25 suppressed errors.
member check passed 1/1.
Check List
Tests
make -C client gotest GOTEST_ARGS='./servicediscovery -count=1'make -C client gotest GOTEST_ARGS='./servicediscovery -race -count=1'make -C client staticThe regression coverage includes degraded-mode eligibility, connection-state transitions, URL replacement, error compatibility, log suppression and recovery, tracker concurrency and allocation behavior, and the end-to-end membership loop.
Benchmarks on darwin/amd64:
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests