Skip to content
Draft
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 112 additions & 10 deletions client/clients/gc/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"context"
"math"
"time"

"github.com/pingcap/errors"
)

// Client is the interface for GC client.
Expand All @@ -32,6 +34,41 @@ type Client interface {
GetGCStatesClient(keyspaceID uint32) GCStatesClient
}

// GCStatesAPIOptions represents all options for GC states API.
type GCStatesAPIOptions struct {
ExcludeGCBarriers bool
ExcludeGlobalGCBarriers bool
}

// DefaultGCStatesAPIOptions returns the default options for GC states API.
func DefaultGCStatesAPIOptions() GCStatesAPIOptions {
return GCStatesAPIOptions{
ExcludeGCBarriers: true,
ExcludeGlobalGCBarriers: true,
}
}

// GCStatesAPIOption is the type of option for GC states API.
type GCStatesAPIOption func(*GCStatesAPIOptions)

// ExcludeGCBarriers controls whether GetGCState and GetAllKeyspacesGCStates should exclude GC barriers.
// Enabling this can reduce the cost of reading GC states, and is recommended for most use cases.
// When GC barriers are needed, explicitly set false to this option.
func ExcludeGCBarriers(whetherToExclude bool) GCStatesAPIOption {
return func(opts *GCStatesAPIOptions) {
opts.ExcludeGCBarriers = whetherToExclude
}
}

// ExcludeGlobalGCBarriers controls whether GetAllKeyspacesGCStates should exclude global GC barriers.
// Enabling this can reduce the cost of reading GC states, and is recommended for most use cases.
// When global GC barriers are needed, explicitly set false to this option.
func ExcludeGlobalGCBarriers(whetherToExclude bool) GCStatesAPIOption {
return func(opts *GCStatesAPIOptions) {
opts.ExcludeGlobalGCBarriers = whetherToExclude
}
}

// GCStatesClient is the interface for users to access GC states.
// KeyspaceID is already bound to this type when created.
//
Expand Down Expand Up @@ -72,7 +109,7 @@ type GCStatesClient interface {
//
// When this method is called on a keyspace without keyspace-level GC enabled, it will be equivalent to calling it on
// the NullKeyspace.
GetGCState(ctx context.Context) (GCState, error)
GetGCState(ctx context.Context, opts ...GCStatesAPIOption) (GCState, error)
// SetGlobalGCBarrier sets a global GC barrier, which blocks GC like how GC barriers do, but is effective for all
// keyspaces. This API is designed for some special needs to block GC of all keyspaces.
//
Expand Down Expand Up @@ -105,11 +142,11 @@ type GCStatesClient interface {
SetGlobalGCBarrier(ctx context.Context, barrierID string, barrierTS uint64, ttl time.Duration) (*GlobalGCBarrierInfo, error)
// DeleteGlobalGCBarrier deletes a global GC barrier.
DeleteGlobalGCBarrier(ctx context.Context, barrierID string) (*GlobalGCBarrierInfo, error)
// Get the GC states from all keyspaces.
// GetAllKeyspacesGCStates gets GC states from all keyspaces.
// The return value includes both GC states and global GC barriers information.
// If a keyspace's state is not ENABLED(like DISABLE/ARCHIVED/TOMBSTONE), that keyspace is skipped.
// If a keyspace is not configured with keyspace level GC, its GCState data is missing.
GetAllKeyspacesGCStates(ctx context.Context) (ClusterGCStates, error)
GetAllKeyspacesGCStates(ctx context.Context, opts ...GCStatesAPIOption) (ClusterGCStates, error)
}

// InternalController is the interface for controlling GC execution.
Expand Down Expand Up @@ -245,16 +282,81 @@ func (b *GlobalGCBarrierInfo) isExpiredImpl(now time.Time) bool {
//nolint:revive
type GCState struct {
// The ID of the keyspace this GC state belongs to.
KeyspaceID uint32
TxnSafePoint uint64
GCSafePoint uint64
GCBarriers []*GCBarrierInfo
KeyspaceID uint32
TxnSafePoint uint64
GCSafePoint uint64
hasGCBarriers bool
gcBarriers []*GCBarrierInfo
}

// NewGCStateWithoutGCBarriers creates a GCState instance without GC barriers info.
func NewGCStateWithoutGCBarriers(keyspaceID uint32, txnSafePoint uint64, gcSafePoint uint64) GCState {
return GCState{
KeyspaceID: keyspaceID,
TxnSafePoint: txnSafePoint,
GCSafePoint: gcSafePoint,
hasGCBarriers: false,
gcBarriers: nil,
}
}

// NewGCStateWithGCBarriers creates a GCState instance with GC barriers info.
func NewGCStateWithGCBarriers(keyspaceID uint32, txnSafePoint uint64, gcSafePoint uint64, gcBarriers []*GCBarrierInfo) GCState {
return GCState{
KeyspaceID: keyspaceID,
TxnSafePoint: txnSafePoint,
GCSafePoint: gcSafePoint,
hasGCBarriers: true,
gcBarriers: gcBarriers,
}
}

func (s GCState) HasGCBarriers() bool {
return s.hasGCBarriers
}

func (s GCState) GetGCBarriers() ([]*GCBarrierInfo, error) {
if !s.HasGCBarriers() {
return nil, errors.New("trying to get GC barriers from GCState that doesn't provide GC barriers info. " +
"to retrieve GC barriers, pass false to excludeGCBarriers parameter to GC APIs")
}
return s.gcBarriers, nil
}

// ClusterGCStates represents the information of the GC state for all keyspaces.
type ClusterGCStates struct {
// Maps from keyspace id to GC state of that keyspace.
GCStates map[uint32]GCState
// All existing global GC barriers.
GlobalGCBarriers []*GlobalGCBarrierInfo
GCStates map[uint32]GCState
hasGlobalGCBarriers bool
globalGCBarriers []*GlobalGCBarrierInfo
}

// NewClusterGCStatesWithoutGlobalGCBarriers creates a ClusterGCStates instance without global GC barriers info.
func NewClusterGCStatesWithoutGlobalGCBarriers(gcStates map[uint32]GCState) ClusterGCStates {
return ClusterGCStates{
GCStates: gcStates,
hasGlobalGCBarriers: false,
globalGCBarriers: nil,
}
}

// NewClusterGCStatesWithGlobalGCBarriers creates a ClusterGCStates instance with global GC barriers info.
func NewClusterGCStatesWithGlobalGCBarriers(gcStates map[uint32]GCState, globalGCBarriers []*GlobalGCBarrierInfo) ClusterGCStates {
return ClusterGCStates{
GCStates: gcStates,
hasGlobalGCBarriers: true,
globalGCBarriers: globalGCBarriers,
}
}

func (s ClusterGCStates) HasGlobalGCBarriers() bool {
return s.hasGlobalGCBarriers
}

func (s ClusterGCStates) GetGlobalGCBarriers() ([]*GlobalGCBarrierInfo, error) {
if !s.HasGlobalGCBarriers() {
return nil, errors.New("trying to get global GC barriers from ClusterGCStates that doesn't provide global GC barriers info. " +
"to retrieve global GC barriers, pass false to excludeGlobalGCBarriers parameter to GC APIs")
}
return s.globalGCBarriers, nil
Comment on lines +320 to +373
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the new accessor API self-describing.

These exported accessors are now part of the public client contract, but they don't have GoDoc, and the error text still tells callers to pass nonexistent exclude... parameters instead of using gc.ExcludeGCBarriers(false) / gc.ExcludeGlobalGCBarriers(false). That leaves both generated docs and runtime guidance out of sync with the option-based API.

As per coding guidelines, "Exported identifiers need GoDoc starting with the name; avoid stutter (pd.PDServer -> Server)."

🤖 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/clients/gc/client.go` around lines 314 - 361, Add GoDoc comments for
the exported symbols (GCState.HasGCBarriers, GCState.GetGCBarriers,
ClusterGCStates, NewClusterGCStatesWithoutGlobalGCBarriers,
NewClusterGCStatesWithGlobalGCBarriers, ClusterGCStates.HasGlobalGCBarriers,
ClusterGCStates.GetGlobalGCBarriers) that start with the symbol name and briefly
describe what they return/represent; avoid stuttering in the type comment (e.g.,
"ClusterGCStates represents the GC state for all keyspaces."). Also update the
error text in GetGCBarriers and GetGlobalGCBarriers to instruct callers to use
the option-based API (mention gc.ExcludeGCBarriers(false) and
gc.ExcludeGlobalGCBarriers(false) respectively) instead of the old exclude...
parameter wording so runtime guidance and generated docs are consistent.

}
39 changes: 39 additions & 0 deletions client/clients/gc/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,42 @@ func TestGCBarrierInfoExpiration(t *testing.T) {
re.False(b1.isExpiredImpl(now))
re.False(b1.isExpiredImpl(now.Add(time.Hour * 24 * 365 * 10)))
}

func TestGCStateAccessors(t *testing.T) {
re := require.New(t)

state := NewGCStateWithoutGCBarriers(1, 2, 3)
re.False(state.HasGCBarriers())
barriers, err := state.GetGCBarriers()
re.Error(err)
re.Nil(barriers)

state = NewGCStateWithGCBarriers(1, 2, 3, []*GCBarrierInfo{
NewGCBarrierInfo("b1", 4, time.Second, time.Now()),
})
re.True(state.HasGCBarriers())
barriers, err = state.GetGCBarriers()
re.NoError(err)
re.Len(barriers, 1)
re.Equal("b1", barriers[0].BarrierID)
}

func TestClusterGCStatesAccessors(t *testing.T) {
re := require.New(t)

state := NewGCStateWithoutGCBarriers(1, 2, 3)
clusterState := NewClusterGCStatesWithoutGlobalGCBarriers(map[uint32]GCState{1: state})
re.False(clusterState.HasGlobalGCBarriers())
barriers, err := clusterState.GetGlobalGCBarriers()
re.Error(err)
re.Nil(barriers)

clusterState = NewClusterGCStatesWithGlobalGCBarriers(map[uint32]GCState{1: state}, []*GlobalGCBarrierInfo{
NewGlobalGCBarrierInfo("b1", 4, time.Second, time.Now()),
})
re.True(clusterState.HasGlobalGCBarriers())
barriers, err = clusterState.GetGlobalGCBarriers()
re.NoError(err)
re.Len(barriers, 1)
re.Equal("b1", barriers[0].BarrierID)
}
49 changes: 32 additions & 17 deletions client/gc_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,19 +295,25 @@ func (c gcStatesClient) DeleteGCBarrier(ctx context.Context, barrierID string) (
}

// GetGCState gets the current GC state.
func (c gcStatesClient) GetGCState(ctx context.Context) (gc.GCState, error) {
func (c gcStatesClient) GetGCState(ctx context.Context, opts ...gc.GCStatesAPIOption) (gc.GCState, error) {
if span := opentracing.SpanFromContext(ctx); span != nil && span.Tracer() != nil {
span = span.Tracer().StartSpan("pdclient.GetGCState", opentracing.ChildOf(span.Context()))
defer span.Finish()
}
start := time.Now()
defer func() { metrics.CmdDurationGetGCState.Observe(time.Since(start).Seconds()) }()

options := gc.DefaultGCStatesAPIOptions()
for _, opt := range opts {
opt(&options)
}

ctx, cancel := context.WithTimeout(ctx, c.client.inner.option.Timeout)
defer cancel()
req := &pdpb.GetGCStateRequest{
Header: c.client.requestHeader(),
KeyspaceScope: wrapKeyspaceScope(c.keyspaceID),
Header: c.client.requestHeader(),
KeyspaceScope: wrapKeyspaceScope(c.keyspaceID),
ExcludeGcBarriers: options.ExcludeGCBarriers,
}
protoClient, ctx := c.client.getClientAndContext(ctx)
if protoClient == nil {
Expand All @@ -319,10 +325,10 @@ func (c gcStatesClient) GetGCState(ctx context.Context) (gc.GCState, error) {
}

gcState := resp.GetGcState()
return pbToGCState(gcState, start), nil
return pbToGCState(gcState, start, options.ExcludeGCBarriers), nil
}

func pbToGCState(pb *pdpb.GCState, reqStartTime time.Time) gc.GCState {
func pbToGCState(pb *pdpb.GCState, reqStartTime time.Time, excludeGCBarriers bool) gc.GCState {
keyspaceID := constants.NullKeyspaceID
if pb.KeyspaceScope != nil {
keyspaceID = pb.KeyspaceScope.KeyspaceId
Expand All @@ -331,12 +337,10 @@ func pbToGCState(pb *pdpb.GCState, reqStartTime time.Time) gc.GCState {
for _, b := range pb.GetGcBarriers() {
gcBarriers = append(gcBarriers, pbToGCBarrierInfo(b, reqStartTime))
}
return gc.GCState{
KeyspaceID: keyspaceID,
TxnSafePoint: pb.GetTxnSafePoint(),
GCSafePoint: pb.GetGcSafePoint(),
GCBarriers: gcBarriers,
if !excludeGCBarriers {
return gc.NewGCStateWithGCBarriers(keyspaceID, pb.GetTxnSafePoint(), pb.GetGcSafePoint(), gcBarriers)
}
return gc.NewGCStateWithoutGCBarriers(keyspaceID, pb.GetTxnSafePoint(), pb.GetGcSafePoint())
}

// SetGlobalGCBarrier sets (creates or updates) a global GC barrier.
Expand Down Expand Up @@ -394,18 +398,25 @@ func (c gcStatesClient) DeleteGlobalGCBarrier(ctx context.Context, barrierID str
}

// GetAllKeyspacesGCStates gets the GC states from all keyspaces.
func (c gcStatesClient) GetAllKeyspacesGCStates(ctx context.Context) (gc.ClusterGCStates, error) {
func (c gcStatesClient) GetAllKeyspacesGCStates(ctx context.Context, opts ...gc.GCStatesAPIOption) (gc.ClusterGCStates, error) {
if span := opentracing.SpanFromContext(ctx); span != nil && span.Tracer() != nil {
span = span.Tracer().StartSpan("pdclient.GetAllKeyspacesGCState", opentracing.ChildOf(span.Context()))
defer span.Finish()
}
start := time.Now()
defer func() { metrics.CmdDurationGetAllKeyspacesGCStates.Observe(time.Since(start).Seconds()) }()

options := gc.DefaultGCStatesAPIOptions()
for _, opt := range opts {
opt(&options)
}

ctx, cancel := context.WithTimeout(ctx, c.client.inner.option.Timeout)
defer cancel()
req := &pdpb.GetAllKeyspacesGCStatesRequest{
Header: c.client.requestHeader(),
Header: c.client.requestHeader(),
ExcludeGcBarriers: options.ExcludeGCBarriers,
ExcludeGlobalGcBarriers: options.ExcludeGlobalGCBarriers,
}
protoClient, ctx := c.client.getClientAndContext(ctx)
if protoClient == nil {
Expand All @@ -417,19 +428,23 @@ func (c gcStatesClient) GetAllKeyspacesGCStates(ctx context.Context) (gc.Cluster
return gc.ClusterGCStates{}, err
}

var ret gc.ClusterGCStates
ret.GCStates = make(map[uint32]gc.GCState, len(resp.GetGcStates()))
gcStates := make(map[uint32]gc.GCState, len(resp.GetGcStates()))
for _, state := range resp.GetGcStates() {
var keyspaceID uint32
if state.KeyspaceScope == nil {
keyspaceID = constants.NullKeyspaceID
} else {
keyspaceID = state.KeyspaceScope.KeyspaceId
}
ret.GCStates[keyspaceID] = pbToGCState(state, start)
gcStates[keyspaceID] = pbToGCState(state, start, options.ExcludeGCBarriers)
}
if options.ExcludeGlobalGCBarriers {
return gc.NewClusterGCStatesWithoutGlobalGCBarriers(gcStates), nil
}

globalGCBarriers := make([]*gc.GlobalGCBarrierInfo, 0, len(resp.GetGlobalGcBarriers()))
for _, barrier := range resp.GetGlobalGcBarriers() {
ret.GlobalGCBarriers = append(ret.GlobalGCBarriers, pbToGlobalGCBarrierInfo(barrier, start))
globalGCBarriers = append(globalGCBarriers, pbToGlobalGCBarrierInfo(barrier, start))
}
return ret, nil
return gc.NewClusterGCStatesWithGlobalGCBarriers(gcStates, globalGCBarriers), nil
}
2 changes: 2 additions & 0 deletions client/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,5 @@ require (
gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

replace github.com/pingcap/kvproto => github.com/MyonKeminta/kvproto v0.0.0-20260512093457-b1cdacbc026b
4 changes: 2 additions & 2 deletions client/go.sum
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
github.com/MyonKeminta/kvproto v0.0.0-20260512093457-b1cdacbc026b h1:NMJKRMjaEtPO0UD8mc5OuZ02AVobD2VAZeBkz0GeHis=
github.com/MyonKeminta/kvproto v0.0.0-20260512093457-b1cdacbc026b/go.mod h1:z6+aAHB7dBkA+LyinEX+48/ImRJ3jag0Hg0c7wkhEvE=
github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA=
github.com/benbjohnson/clock v1.3.0 h1:ip6w0uFQkncKQ979AypyG0ER7mqUSBdKLOgAle/AT8A=
github.com/benbjohnson/clock v1.3.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA=
Expand Down Expand Up @@ -53,8 +55,6 @@ github.com/pingcap/errors v0.11.5-0.20211224045212-9687c2b0f87c h1:xpW9bvK+HuuTm
github.com/pingcap/errors v0.11.5-0.20211224045212-9687c2b0f87c/go.mod h1:X2r9ueLEUZgtx2cIogM0v4Zj5uvvzhuuiu7Pn8HzMPg=
github.com/pingcap/failpoint v0.0.0-20240528011301-b51a646c7c86 h1:tdMsjOqUR7YXHoBitzdebTvOjs/swniBTOLy5XiMtuE=
github.com/pingcap/failpoint v0.0.0-20240528011301-b51a646c7c86/go.mod h1:exzhVYca3WRtd6gclGNErRWb1qEgff3LYta0LvRmON4=
github.com/pingcap/kvproto v0.0.0-20260326084500-678ff92b1edd h1:FA2DzGly3tuBWFjktkJxmqeOVEqgrsUvKMQXAw9xvWE=
github.com/pingcap/kvproto v0.0.0-20260326084500-678ff92b1edd/go.mod h1:z6+aAHB7dBkA+LyinEX+48/ImRJ3jag0Hg0c7wkhEvE=
github.com/pingcap/log v1.1.1-0.20221110025148-ca232912c9f3 h1:HR/ylkkLmGdSSDaD8IDP+SZrdhV1Kibl9KrHxJ9eciw=
github.com/pingcap/log v1.1.1-0.20221110025148-ca232912c9f3/go.mod h1:DWQW5jICDR7UJh4HtxXSM20Churx4CQL0fwL/SoOSA4=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,5 @@ require (
// which will cause several different tests to fail. So this is a temporary workaround to use the old version of `testify`.
// TODO: fix those flasky tests introduced by the behavior change of `Eventually` and `EventuallyWithT` assertions.
replace github.com/stretchr/testify => github.com/stretchr/testify v1.10.0

replace github.com/pingcap/kvproto => github.com/MyonKeminta/kvproto v0.0.0-20260512093457-b1cdacbc026b
Loading