Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: support use API context to create client #6482

Merged
merged 5 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
84 changes: 77 additions & 7 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const (
// defaultKeySpaceGroupID is the default key space group id.
// We also reserved 0 for the keyspace group for the same purpose.
defaultKeySpaceGroupID = uint32(0)
defaultKeyspaceName = "DEFAULT"
)

// Region contains information of a region's meta and its peers.
Expand Down Expand Up @@ -335,6 +336,7 @@ func NewClientWithContext(
}

// NewClientWithKeyspace creates a client with context and the specified keyspace id.
// And now, it's only for test purpose.
func NewClientWithKeyspace(
ctx context.Context, keyspaceID uint32, svrAddrs []string,
security SecurityOption, opts ...ClientOption,
Expand Down Expand Up @@ -388,19 +390,87 @@ func createClientWithKeyspace(
return c, nil
}

// NewClientWithKeyspaceName creates a client with context and the specified keyspace name.
func NewClientWithKeyspaceName(
// APIVersion is the API version the server and the client is using.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// APIVersion is the API version the server and the client is using.
// APIVersion is the API version used by the server and the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are almost the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

nope. The red one has typo. Alternative "APIVersion is the API version that the server and the client are using."

// See more details in https://github.com/tikv/rfcs/blob/master/text/0069-api-v2.md#kvproto
type APIVersion int

// The API versions the client supports.
Copy link
Contributor

Choose a reason for hiding this comment

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

@iosmanthus Is this suitable for the design of APIVersion?

// As for V1TTL, client won't use it and we just remove it.
const (
V1 APIVersion = iota
_
V2
)

// APIContext is the context for API version.
type APIContext interface {
GetAPIVersion() (apiVersion APIVersion)
GetKeyspaceName() (keyspaceName string)
}

type apiContextV1 struct{}

// NewAPIContextV1 creates a API context for V1.
func NewAPIContextV1() APIContext {
return &apiContextV1{}
}

// GetAPIVersion returns the API version.
func (apiCtx *apiContextV1) GetAPIVersion() (version APIVersion) {
return V1
}

// GetKeyspaceName returns the keyspace name.
func (apiCtx *apiContextV1) GetKeyspaceName() (keyspaceName string) {
return ""
}

type apiContextV2 struct {
keyspaceName string
}

// NewAPIContextV2 creates a API context with the specified keyspace name for V2.
func NewAPIContextV2(keyspaceName string) APIContext {
if len(keyspaceName) == 0 {
keyspaceName = defaultKeyspaceName
}
return &apiContextV2{keyspaceName: keyspaceName}
rleungx marked this conversation as resolved.
Show resolved Hide resolved
}

// GetAPIVersion returns the API version.
func (apiCtx *apiContextV2) GetAPIVersion() (version APIVersion) {
return V2
}

// GetKeyspaceName returns the keyspace name.
func (apiCtx *apiContextV2) GetKeyspaceName() (keyspaceName string) {
return apiCtx.keyspaceName
}

// NewClientWithAPIContext creates a client according to the API context.
func NewClientWithAPIContext(
ctx context.Context, apiCtx APIContext, svrAddrs []string,
security SecurityOption, opts ...ClientOption,
) (Client, error) {
apiVersion, keyspaceName := apiCtx.GetAPIVersion(), apiCtx.GetKeyspaceName()
switch apiVersion {
case V1:
return NewClientWithContext(ctx, svrAddrs, security, opts...)
case V2:
return newClientWithKeyspaceName(ctx, keyspaceName, svrAddrs, security, opts...)
default:
return nil, errors.Errorf("[pd] invalid API version %d", apiVersion)
}
}

// newClientWithKeyspaceName creates a client with context and the specified keyspace name.
func newClientWithKeyspaceName(
ctx context.Context, keyspaceName string, svrAddrs []string,
security SecurityOption, opts ...ClientOption,
) (Client, error) {
log.Info("[pd] create pd client with endpoints and keyspace",
zap.Strings("pd-address", svrAddrs), zap.String("keyspace-name", keyspaceName))

Copy link
Contributor

Choose a reason for hiding this comment

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

How about move if keyspaceName == "" logic to here?
newClientWithKeyspaceName is only used for apiv2;
If someone else calls this function, there is no need to repeat the judgment logic

Copy link
Member Author

Choose a reason for hiding this comment

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

#6482 (comment) may be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's not exposed, IMO, we don't need to handle the API logic.

// if keyspace name is empty, fall back to the legacy API
if len(keyspaceName) == 0 {
return NewClientWithContext(ctx, svrAddrs, security, opts...)
}

tlsCfg := &tlsutil.TLSConfig{
CAPath: security.CAPath,
CertPath: security.CertPath,
Expand Down
8 changes: 4 additions & 4 deletions tests/integrations/mcs/testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ func InitLogger(cfg *tso.Config) (err error) {
return err
}

// SetupClientWithDefaultKeyspaceName creates a TSO client with default keyspace name for test.
func SetupClientWithDefaultKeyspaceName(
ctx context.Context, re *require.Assertions, endpoints []string, opts ...pd.ClientOption,
// SetupClientWithAPIContext creates a TSO client with api context name for test.
func SetupClientWithAPIContext(
ctx context.Context, re *require.Assertions, apiCtx pd.APIContext, endpoints []string, opts ...pd.ClientOption,
) pd.Client {
cli, err := pd.NewClientWithKeyspaceName(ctx, "", endpoints, pd.SecurityOption{}, opts...)
cli, err := pd.NewClientWithAPIContext(ctx, apiCtx, endpoints, pd.SecurityOption{}, opts...)
re.NoError(err)
return cli
}
Expand Down
2 changes: 1 addition & 1 deletion tests/integrations/mcs/tso/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func checkTSOPath(re *require.Assertions, isAPIServiceMode bool) {
_, cleanup := mcs.StartSingleTSOTestServer(ctx, re, backendEndpoints, tempurl.Alloc())
defer cleanup()

cli := mcs.SetupClientWithDefaultKeyspaceName(ctx, re, []string{backendEndpoints})
cli := mcs.SetupClientWithAPIContext(ctx, re, pd.NewAPIContextV2(""), []string{backendEndpoints})
physical, logical, err := cli.GetTS(ctx)
re.NoError(err)
ts := tsoutil.ComposeTS(physical, logical)
Expand Down
2 changes: 1 addition & 1 deletion tests/integrations/tso/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ func checkTSO(ctx context.Context, re *require.Assertions, wg *sync.WaitGroup, b
for i := 0; i < tsoRequestConcurrencyNumber; i++ {
go func() {
defer wg.Done()
cli := mcs.SetupClientWithDefaultKeyspaceName(ctx, re, strings.Split(backendEndpoints, ","))
cli := mcs.SetupClientWithAPIContext(ctx, re, pd.NewAPIContextV1(), strings.Split(backendEndpoints, ","))
var ts, lastTS uint64
for {
select {
Expand Down
Loading