pd-ctl: add GC state inspection commands - #11054
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
|
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:
📝 WalkthroughWalkthroughAdds keyspace-level GC metadata propagation and introduces the read-only ChangesGC state inspection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant GCStateCommand
participant PDGCStateReader
participant PD
Operator->>GCStateCommand: invoke keyspace, global, or all with include-expired option
GCStateCommand->>PDGCStateReader: request GC state
PDGCStateReader->>PD: call GC state RPC
PD-->>PDGCStateReader: return states and barriers
PDGCStateReader-->>GCStateCommand: return state data
GCStateCommand-->>Operator: write filtered sorted JSON
Possibly related PRs
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 |
ti-chi-bot
Bot
added
the
size/XXL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11054 +/- ##
==========================================
+ Coverage 79.43% 79.47% +0.03%
==========================================
Files 542 543 +1
Lines 77117 77466 +349
==========================================
+ Hits 61259 61566 +307
- Misses 11571 11584 +13
- Partials 4287 4316 +29
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/pd-ctl/pdctl/command/gc_state_command.go (1)
344-352: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPass the original error to
status.Code.
status.FromErrorunwraps viaerrors.As, sostatus.Code(err)already coversfmt.Errorf("%w")chains;errors.Cause(err)only followspingcap/errorscauser chains and can miss gRPC status errors wrapped outside that chain. Apply this fix to the duplicateUnimplementedchecks as well.🤖 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 `@tools/pd-ctl/pdctl/command/gc_state_command.go` around lines 344 - 352, Update the Unimplemented status check in the GC-state retrieval error path to pass the original err directly to status.Code instead of errors.Cause(err), preserving the existing annotation behavior. Apply the same change to every duplicate Unimplemented check in this command.
🤖 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 `@tools/pd-ctl/pdctl/command/gc_state_command.go`:
- Around line 197-208: Update newLocalGCBarrierOutputs to skip nil barrier
entries before accessing BarrierID, BarrierTS, or TTL. Preserve the existing
output construction and sorting for non-nil entries so malformed protobuf
responses do not panic the CLI.
In `@tools/pd-ctl/tests/safepoint/gc_state_test.go`:
- Around line 83-88: Widen the lower TTL bound in the expires branch of the
safepoint test from 3595 seconds to a more tolerant value, while keeping the
3600-second upper bound and the non-expiring MaxInt64 assertion unchanged.
---
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/gc_state_command.go`:
- Around line 344-352: Update the Unimplemented status check in the GC-state
retrieval error path to pass the original err directly to status.Code instead of
errors.Cause(err), preserving the existing annotation behavior. Apply the same
change to every duplicate Unimplemented check in this command.
🪄 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: e91048ab-3bb5-41ce-83f7-160a2afa5049
📒 Files selected for processing (10)
client/clients/gc/client.goclient/gc_client.goclient/gc_client_test.gopkg/storage/endpoint/cluster_id_test.gotests/integrations/client/client_test.gotools/pd-ctl/README.mdtools/pd-ctl/pdctl/command/gc_state_command.gotools/pd-ctl/pdctl/command/gc_state_command_test.gotools/pd-ctl/pdctl/ctl.gotools/pd-ctl/tests/safepoint/gc_state_test.go
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
and removed
do-not-merge/needs-linked-issue
labels
| if barrier == nil { | ||
| continue | ||
| } | ||
| result = append(result, gcBarrierOutput{ |
There was a problem hiding this comment.
When an expired barrier has not yet been lazily deleted by a later safe-point advancement, the read RPCs still return it with a zero TTL, and both projection helpers include it unconditionally. gc-state therefore mixes inactive local and global barriers with current blockers, so operators and scripts can attribute a stalled safe point to the wrong barrier.
There was a problem hiding this comment.
Confirmed. I’ll omit zero-TTL barriers by default so gc-state shows effective blockers. However, expired barriers that are still awaiting lazy deletion are useful for diagnosing what previously blocked GC and whether a later safe-point advancement has performed cleanup. I’ll therefore add an --include-expired flag for keyspace, global, and all; when enabled, these barriers will remain in the existing arrays with ttl_seconds: 0.
|
/retest |
|
/retest |
|
/check-issue-triage-complete |
| client clusterGCStatesClient, | ||
| excludeGCBarriers bool, | ||
| ) (gc.ClusterGCStates, error) { | ||
| return client.GetAllKeyspacesGCStates( |
There was a problem hiding this comment.
gc-state global reaches this GetAllKeyspacesGCStates call even though it only emits global barriers. On clusters with many keyspaces, PD still scans and returns every keyspace state, the client materializes the full map and then discards it, and the default three-second client timeout can make this diagnostic fail while imposing unnecessary PD/etcd work.
There was a problem hiding this comment.
This is intentional. gc-state global is a diagnostic projection over the existing GetAllKeyspacesGCStates response rather than a new production API. We currently don’t have a concrete consumer or production requirement for a global-barriers-only RPC, so adding new kvproto, server, and client API surface solely for this low-frequency diagnostic command would be premature.
The command also sets ExcludeGCBarriers(true). On the PD leader, this lets GetAllKeyspacesGCStates reuse the GC-state cache and avoids loading every keyspace’s local barriers or running the full GC-state storage path on cache hits. Keyspace metadata is still enumerated and the safe-point states are still returned, but that remaining cost is an intentional trade-off for reusing the existing compatible RPC. We can revisit a global-only API if production usage or measurements show that this path cannot meet the diagnostic command’s latency requirements.
There was a problem hiding this comment.
Addressed by adding opt-in global-barrier support to GetGCState in #11117 and reworking this command in 90d0453. The standalone gc-state global command has been removed. gc-state keyspace now fetches the requested keyspace state and global barriers with a single GetGCState call; GetAllKeyspacesGCStates is used only by gc-state all, where full enumeration is expected.
|
|
||
| type gcStateReader interface { | ||
| getGCState(context.Context, uint32) (gc.GCState, error) | ||
| getGlobalGCState(context.Context) (gc.ClusterGCStates, error) |
There was a problem hiding this comment.
getGlobalGCState and getAllKeyspacesGCStates are two reader methods for the same RPC; the only behavioral difference is the ExcludeGCBarriers boolean passed below. Keeping both names duplicates the interface, fake, and option-test surface and makes the global command look cheaper than the full-cluster read it actually performs.
There was a problem hiding this comment.
The shared RPC is intentional, but gcStateReader is the command-facing abstraction rather than a direct mirror of the PD client API. getGlobalGCState and getAllKeyspacesGCStates represent two different command behaviors even though the current adapter implements both with GetAllKeyspacesGCStates.
Keeping them separate lets the command and its tests depend on those behaviors instead of depending on ExcludeGCBarriers or another transport-level parameter. The concrete reader already shares the common RPC implementation through readClusterGCStates, so the actual implementation duplication is limited. It also lets the adapter change independently if the underlying APIs diverge later, without changing command routing or command tests.
The cost of reusing GetAllKeyspacesGCStates for the global diagnostic view is intentional and is discussed as before; the method name describes the state requested by the command rather than promising a dedicated RPC.
| } { | ||
| t.Run(testCase.name, func(t *testing.T) { | ||
| pbState := &pdpb.GCState{ | ||
| KeyspaceScope: &pdpb.KeyspaceScope{KeyspaceId: 42}, |
There was a problem hiding this comment.
Current master no longer has the direct KeyspaceId field after the API-v3 protobuf migration, and the same migration removed the direct .Id accesses used in gc_state_test.go. The clean merge therefore leaves newly added tests that do not compile, so the pre-migration CI result does not validate the current target branch.
There was a problem hiding this comment.
Thanks for the heads-up. I’ll rebase onto the latest master and adapt the affected code to the API v3 changes.
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
7bc7ad3 to
0696343
Compare
|
/retest |
Remove the one-time design and implementation workflow documents now that the GC state command changes are implemented. Keep the PR focused on durable product code, tests, and user documentation. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
| } | ||
| defer reader.close() | ||
|
|
||
| clusterState, err := reader.getAllKeyspacesGCStates( |
There was a problem hiding this comment.
GetAllKeyspacesGCStates scans every keyspace and loads each keyspace-level scope's local barriers, while this client keeps the default three-second RPC timeout. At sufficiently large keyspace or barrier cardinality, gc-state all can time out before returning any diagnostic output, even though full-cluster inspection is the purpose of this subcommand.
There was a problem hiding this comment.
Fixed. gc-state now uses a configurable --timeout with a 30-second default and passes it to the dedicated PD client through opt.WithCustomTimeoutOption.
Use a 30-second default and expose --timeout on both GC state commands. This lets full-cluster inspection tolerate large keyspace and barrier sets. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
|
/retest |
ti-chi-bot
Bot
added
needs-1-more-lgtm
There was a problem hiding this comment.
The markdown in this document has strange, premature line breaks. I don't think they're necessary.
|
|
||
| Use `--exclude-global-barriers` to skip the global-barrier read and remove the | ||
| `global_gc_barriers` field from the JSON output. The flag applies to both | ||
| remaining subcommands: |
There was a problem hiding this comment.
This wording depends on the history of the removed global subcommand. Please describe the current interface directly: The flag applies to both subcommands.
| } | ||
| } | ||
|
|
||
| func TestGCStateGlobalCommandIsRemoved(t *testing.T) { |
There was a problem hiding this comment.
Please name this test after the current behavior rather than the removed implementation, for example TestGCStateRejectsGlobalSubcommand.
There was a problem hiding this comment.
Removed this test instead. The final interface is already covered by TestGCStateCommandHelpContract, which asserts that the supported subcommands are exactly keyspace and all.
| return result | ||
| } | ||
|
|
||
| func newGlobalGCBarrierOutputs(barriers []*gc.GlobalGCBarrierInfo, includeExpired bool) []gcBarrierOutput { |
There was a problem hiding this comment.
Please consolidate these two identical filter, projection, and sort pipelines so local and global barrier output cannot drift.
There was a problem hiding this comment.
Nice catch! Consolidated filtering, projection, TTL conversion, and sorting into a shared helper. The local and global functions now only adapt their distinct input types.
Share local and global barrier output conversion to prevent behavior drift. Update the guide for the final command surface and remove the history-only test. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
| } | ||
| excludedNullStateWithExpired, ok := excludedStatesByIDWithExpired[constant.NullKeyspaceID] | ||
| re.True(ok) | ||
| re.Equal(nullState, excludedNullStateWithExpired) |
There was a problem hiding this comment.
These equality checks compare snapshots from separate command executions, including finite TTLs that decrease over time. The test can cross a one-second boundary and fail even when the state is otherwise unchanged (I reproduced 3600 vs 3599). Please compare only the stable fields and validate barriers with requireGCStateCommandBarriers; the same applies to the other whole-state equality checks in this block.
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JmPotato, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
3 participants
What problem does this PR solve?
PD exposes per-keyspace and cluster-wide GC state through RPCs, but operators cannot inspect that state through
pd-ctl. This makes it difficult to identify whether GC advancement is blocked by a keyspace-local barrier or a cluster-wide global barrier.Issue Number: close #11013, ref #8978
What is changed and how does it work?
Check List
Tests
Release note