pd-ctl: add GC state inspection commands - #11054
Conversation
|
Skipping CI for Draft Pull Request. |
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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 💡 1🛠️ Fix failing CI checks 💡
🧪 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.23% 79.34% +0.11%
==========================================
Files 540 542 +2
Lines 76010 76639 +629
==========================================
+ Hits 60226 60813 +587
- Misses 11533 11555 +22
- Partials 4251 4271 +20
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
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 |
Present only effective GC scopes in gc-state all and use the NullKeyspace scope for unified GC keyspaces. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
Skip nil local and global barrier entries before projecting JSON, and use gRPC status unwrapping directly for compatibility errors. Relax the finite TTL assertion to avoid slow-CI flakes. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
|
/retest |
Show correlated local and global barriers across the keyspace, global, and combined GC-state views. Clarify NullKeyspace placement, empty arrays, and finite versus non-expiring TTLs. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
|
/retest |
2 similar comments
|
/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.
Keep the effective GC state focused on active barriers while allowing operators to inspect zero-TTL entries with --include-expired. Signed-off-by: Wenxuan Zhang <wenxuangm@gmail.com>
|
/retest |
|
/retest |
2 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 identifywhether 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?
Add read-only
pd-ctl gc-state keyspace,gc-state global, andgc-state allcommands.Expose the server-provided keyspace-level GC mode through the public GC client
model so the command can distinguish independent keyspace GC from unified GC.
Return deterministic JSON containing effective transaction and GC safe points,
local GC barriers, and cluster-wide global GC barriers. Keep local barriers in
per-keyspace results and report global barriers only in the global view or once
at the top level of the combined view.
Document the troubleshooting workflow and cover the command with unit,
Classic/NextGen integration, help-contract, and TiDB-X playground tests.
Check List
Tests
lifecycle, error handling, and help contracts
pd-ctlcommand execution and legacy command regression coveragegc-state,keyspace,global, andallhelp outputand verified their placement and values in all three JSON views
make static,make pd-ctl, andNEXT_GEN=1 make pd-server pd-ctlRelease note
Summary by CodeRabbit
Summary by CodeRabbit
pd-ctl gc-statewithkeyspace,global, andallviews.IsKeyspaceLevelGC).--include-expiredto control visibility of zero-TTL (expired) GC barriers in JSON output.