gc: Support excluding GC barriers and global GC barriers when retrieving GC states#10669
gc: Support excluding GC barriers and global GC barriers when retrieving GC states#10669MyonKeminta wants to merge 9 commits into
Conversation
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
ti-chi-bot
Bot
added
release-note-none
|
[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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds option-driven GC-barrier exclusion: new GCStatesAPIOptions, guarded GCState/ClusterGCStates accessors and constructors, client/server wiring for exclusion flags, state-manager reads that optionally omit barriers (and separate singleflight paths), endpoint updates, and comprehensive test changes. ChangesGC Barrier Exclusion API
Sequence Diagram(s) sequenceDiagram
participant Client
participant PDServer
participant GCStateManager
Client->>PDServer: GetAllKeyspacesGCStates(opts: ExcludeGcBarriers/ExcludeGlobalGcBarriers)
PDServer->>GCStateManager: GetAllKeyspacesGCStates(ctx, excludeGCBarriers)
GCStateManager-->>PDServer: ClusterGCStates (with or without global barriers)
PDServer-->>Client: ClusterGCStates response
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
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
🤖 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 `@client/clients/gc/client.go`:
- Around line 314-362: Add GoDoc comments for the exported accessors: place a
comment starting with the exact function name above HasGCBarriers and
GetGCBarriers on type GCState and above HasGlobalGCBarriers and
GetGlobalGCBarriers on type ClusterGCStates; each comment should begin with the
function name, briefly describe what it returns (e.g., whether GC
barriers/global GC barriers are available) and for Get* methods document the
returned value and the error condition when the corresponding Has* is false.
Ensure the comments follow GoDoc style (start with the identifier) and mention
the types GCState and ClusterGCStates where appropriate.
In `@pkg/gc/gc_state_manager.go`:
- Around line 703-709: GetAllKeyspacesGCStates currently uses
m.allKeyspacesGCStatesSingleFlight.Do without incorporating the
excludeGCBarriers flag into the singleflight key, so concurrent calls with
different excludeGCBarriers values can return the wrong shape; fix by including
excludeGCBarriers in the singleflight key used when calling Do (e.g. append a
":barriers=true/false" suffix or otherwise derive a unique key per
excludeGCBarriers) and keep calling getAllKeyspacesGCStatesImpl(execCtx,
excludeGCBarriers) inside the lambda so each distinct key has its own
in-flight/result.
🪄 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
Run ID: 04a38f0a-4825-48c2-85b0-de1330aed450
⛔ Files ignored due to path filters (4)
client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumtests/integrations/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
client/clients/gc/client.goclient/clients/gc/client_test.goclient/gc_client.goclient/go.modgo.modpkg/gc/gc_state_manager.gopkg/gc/gc_state_manager_test.goserver/api/service_gc_safepoint.goserver/gc_service.gotests/integrations/client/client_test.gotests/integrations/go.modtests/server/api/service_gc_safepoint_test.gotests/server/gc/gc_test.gotools/go.mod
| func (m *GCStateManager) GetAllKeyspacesGCStates(ctx context.Context, excludeGCBarriers bool) (map[uint32]GCState, error) { | ||
| return m.allKeyspacesGCStatesSingleFlight.Do(ctx, func(execCtx context.Context) (map[uint32]GCState, error) { | ||
| result, err := m.getAllKeyspacesGCStatesImpl(execCtx) | ||
| result, err := m.getAllKeyspacesGCStatesImpl(execCtx, excludeGCBarriers) |
There was a problem hiding this comment.
singleflight.Do uses ctx as the dedup key, but excludeGCBarriers is not included in the key. When two concurrent calls pass different excludeGCBarriers values, only one lambda will execute, and the other caller will get a result of the wrong shape (for example, caller A passes excludeGCBarriers=false expecting to get GC barriers, but after dedup, it gets the result from caller B who passed excludeGCBarriers=true — no barriers).
There was a problem hiding this comment.
It's not the builtin singleflight but the hand-written linearizable version OrderedSingleFlight, and it doesn't support key yet.
I added a separated singleflight fir it.
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
| } | ||
|
|
||
| // DefaultGCStatesAPIOptions returns the default options for GC states API. | ||
| func DefaultGCStatesAPIOptions() GCStatesAPIOptions { |
There was a problem hiding this comment.
This changes the Go client default behavior: existing callers of GetGCState/GetAllKeyspacesGCStates will silently stop receiving barriers. Can we keep the old default and make exclusion opt-in, or call this out as a breaking change?
There was a problem hiding this comment.
As it's hard to push all callers to update their usage, I suggest let this be a breaking change. Callers that really needs the GC barriers will receive compile error, and is forced to switch to the safe retriver function; then if they missed the new options, they will get an error.
Actually, I expect that there's no such usage for now. If so, nothing will happen after upgrading the PD client.
| KeyspaceID uint32 | ||
| TxnSafePoint uint64 | ||
| GCSafePoint uint64 | ||
| hasGCBarriers bool |
There was a problem hiding this comment.
Removing the exported barrier fields breaks source compatibility for existing users. Could we keep exported fields for compatibility and add Has/Get helpers to distinguish not-fetched from an empty result?
There was a problem hiding this comment.
Ditto. And this prevents misuse.
| TxnSafePoint: pb.GetTxnSafePoint(), | ||
| GCSafePoint: pb.GetGcSafePoint(), | ||
| GCBarriers: gcBarriers, | ||
| if !excludeGCBarriers { |
There was a problem hiding this comment.
Before this branch, we already allocate and convert all returned barriers. During rolling upgrades, an old PD may still return barriers even when the new client requested exclusion, so we can return early before the conversion.
3 participants
What problem does this PR solve?
Issue Number: Ref #10659
What is changed and how does it work?
Check List
Tests
Code changes
-
Side effects
Related changes
-
Release note
Summary by CodeRabbit
New Features
Refactor
Tests
Chores