Introduce API v2 #494

iosmanthus · 2022-05-12T09:41:11Z

Signed-off-by: iosmanthus [email protected]

Introducing API V2 for client-go, all the interfaces of RawKV have been upgraded, and their tests are migrated to detect the API version of the cluster.

Migrate RawKV tests to auto-detect API version.

Signed-off-by: iosmanthus <[email protected]>

pingyu · 2022-05-12T13:31:03Z

kv/api_version.go

+	return append(GetV2Prefix(mode), key...)
+}
+
+func DecodeV2StartKey(mode Mode, key []byte) []byte {


Suggest to combine the following methods as DecodeV2KeyRange. Then it would be easier to be used as startKey, endKey := DecodeV2KeyRange(startKey, endKey).

pingyu · 2022-05-12T13:31:58Z

kv/api_version.go

+func DecodeV2StartKey(mode Mode, key []byte) []byte {
+	minKey := GetV2Prefix(mode)
+	if bytes.Compare(key, minKey) < 0 {
+		return []byte{}


Suggest to return error, other than return an unexpected value silently.

pingyu · 2022-05-12T13:38:37Z

rawkv/rawkv.go

@@ -176,7 +198,7 @@ func (c *Client) Get(ctx context.Context, key []byte, options ...RawOption) ([]b
 	req := tikvrpc.NewRequest(
 		tikvrpc.CmdRawGet,
 		&kvrpcpb.RawGetRequest{
-			Key: key,
+			Key: c.buildRequestKey(key),


Then we will need to encode keys twice (one for request PD & another for request TiKV) ? How about do the encoding before here. And then it also seems that we don't need the CodecPDClientV2.

pingyu · 2022-05-12T13:52:17Z

rawkv/rawkv.go

+	return key
+}
+
+func (c *Client) buildRequestEndKey(key []byte) []byte {


Suggest to implement this method as buildRequestKeyRange, as "end key" & empty string only make sense in a range.

Signed-off-by: iosmanthus <[email protected]>

sunxiaoguang · 2022-06-07T09:39:40Z

internal/client/api_version.go

+)
+
+var (
+	ApiV2RawKeyPrefix = []byte("r")


We need to add prefix reserved for keyspace now.

sunxiaoguang · 2022-06-07T09:52:30Z

internal/locate/region_cache.go

@@ -483,6 +484,15 @@ func (c *RegionCache) SetPDClient(client pd.Client) {
 	c.pdClient = client
 }

+func (c *RegionCache) GetApiVersion() kvrpcpb.APIVersion {


why not store api version when creating RegionCache?

ApiVersion should be removed from RegionCache, and moved to RegionRequestSender. The API version could be derived while constructing RegionRequestSender.

sunxiaoguang · 2022-06-07T09:59:14Z

rawkv/rawkv.go

@@ -756,7 +777,7 @@ func (c *Client) sendBatchPut(bo *retry.Backoffer, keys, values [][]byte, ttls [
 	var batches []kvrpc.Batch
 	// split the keys by size and RegionVerID
 	for regionID, groupKeys := range groups {
-		batches = kvrpc.AppendBatches(batches, regionID, groupKeys, keyToValue, keyTottl, rawBatchPutSize)
+		batches = kvrpc.AppendBatches(batches, regionID, groupKeys, keyToValue, keyToTTL, rawBatchPutSize)


We can combine keyToValue and keyToTTL to a single map to save memory and lookup

Signed-off-by: iosmanthus <[email protected]>

pingyu · 2022-06-13T06:44:08Z

rawkv/rawkv.go

@@ -148,6 +149,26 @@ func NewClient(ctx context.Context, pdAddrs []string, security config.Security,
 	}, nil
 }

+// NewClientV2 creates a client with PD cluster addrs with api version of v2.
+func NewClientV2(ctx context.Context, pdAddrs []string, security config.Security, opts ...pd.ClientOption) (*Client, error) {


How about support API v2 by options, and add a new constructor (e.g., NewClientOpt) to accept the opts ...RawOption ?

The opts ...RawOption on constructing Client is required, when recently I'm trying to solve a proxy issue. I find that it's difficult to assign WithContextDialer to underlying gRPC connections. Compared with pd/client which has pd ClientOptions and a WithGRPCDialOptions method, , it's quite easy to do so.

Usage example of pd client with gRPC dialer:

dialProxy, err := proxy.SOCKS5("tcp", "127.0.0.1:1080", nil, proxy.Direct) grpcOpt := grpc.WithContextDialer(func(ctx context.Context, address string) (net.Conn, error) { return dialProxy.(proxy.ContextDialer).DialContext(ctx, "tcp", address) }) pdCli, err := pd.NewClient(pdAddrs, pd.SecurityOption{ CAPath: security.ClusterSSLCA, CertPath: security.ClusterSSLCert, KeyPath: security.ClusterSSLKey, }, pd.WithGRPCDialOptions(grpcOpt))

Signed-off-by: iosmanthus <[email protected]>

pingyu · 2022-06-14T10:35:08Z

internal/client/client.go

@@ -121,21 +121,22 @@ type connArray struct {
 	done chan struct{}
 }

-func newConnArray(maxSize uint, addr string, security config.Security, idleNotify *uint32, enableBatch bool, dialTimeout time.Duration) (*connArray, error) {
+func newConnArray(maxSize uint, addr string, security config.Security,
+	idleNotify *uint32, enableBatch bool, dialTimeout time.Duration, opts ...grpc.DialOption) (*connArray, error) {


Suggest to use opts []grpc.DialOption instead, in case we will have more argument later.

Make sense to me, since it's a private internal function.

pingyu · 2022-06-14T10:37:28Z

rawkv/rawkv.go

@@ -131,20 +166,32 @@ func (c *Client) SetColumnFamily(columnFamily string) *Client {
 }

 // NewClient creates a client with PD cluster addrs.
-func NewClient(ctx context.Context, pdAddrs []string, security config.Security, opts ...pd.ClientOption) (*Client, error) {
+func NewClient(ctx context.Context, pdAddrs []string, opts ...ClientOpt) (*Client, error) {


I'm not sure whether it is a good idea to change method arguments and break compatibility. Maybe we can add a new method and implement NewClient by the new one.

How about creating a new function called NewClientWithOpts

Signed-off-by: iosmanthus <[email protected]>

pingyu

The NewClientWithOpts LGTM~

disksing · 2022-06-21T09:03:00Z

tikv/region.go

@@ -91,6 +91,17 @@ type RPCRuntimeStats = locate.RPCRuntimeStats
 // CodecPDClient wraps a PD Client to decode the encoded keys in region meta.
 type CodecPDClient = locate.CodecPDClient

+type CodecPDClientV2 = locate.CodecPDClientV2


need some comment.

disksing · 2022-06-21T09:10:58Z

rawkv/rawkv.go

@@ -37,6 +37,7 @@ package rawkv
 import (
 	"bytes"
 	"context"
+	"google.golang.org/grpc"


not in good order

internal/locate/region_request.go

Signed-off-by: iosmanthus <[email protected]>

internal/client/api_version.go

tikvrpc/tikvrpc.go

sunxiaoguang · 2022-07-06T15:13:55Z

internal/locate/region_cache.go

-			var err error
+		var err error
+		oldMeta := meta
+		switch c.pdClient.(type) {


Why not move decodeRegionMetaKeyWithShallowCopy into interface and use interface to dispatch.

Because c.pdClient is a pd.Client interface already, if we change the interface, we need to modify the PD.

sunxiaoguang · 2022-07-06T15:15:56Z

internal/locate/region_request.go

@@ -189,8 +190,18 @@ func RecordRegionRequestRuntimeStats(stats map[tikvrpc.CmdType]*RPCRuntimeStats,

 // NewRegionRequestSender creates a new sender.
 func NewRegionRequestSender(regionCache *RegionCache, client client.Client) *RegionRequestSender {
+	var v kvrpcpb.APIVersion
+
+	switch regionCache.pdClient.(type) {


Type assertion looks verbose, we can cache the version somewhere in RegionCache or simply have a method in client to return API version.

We could store the API version in the region cache, however, the type assertion is still needed while constructing the region cache. By this lifting, we could reduce type assertion in runtime, since the region cache construction happens only once for each client.

Signed-off-by: iosmanthus <[email protected]>

iosmanthus · 2022-07-07T10:16:33Z

@sticnarf @sunxiaoguang @disksing PTAL

Signed-off-by: iosmanthus <[email protected]>

sunxiaoguang · 2022-07-07T10:59:34Z

internal/locate/region_cache.go

-			var err error
+		var err error
+		oldMeta := meta
+		switch c.pdClient.(type) {


integration_tests/raw/api_test.go

Signed-off-by: iosmanthus <[email protected]>

sunxiaoguang

LGTM

sunxiaoguang · 2022-07-09T06:49:41Z

Compatibility test will be fixed in a separate PR.

iosmanthus added 3 commits April 28, 2022 22:03

introduce raw.Client with api v2

80a8a2a

Signed-off-by: iosmanthus <[email protected]>

Merge branch 'master' of github.com:tikv/client-go into api-v2

6ca83f9

migrate rawkv test to api v2

b442b3c

Signed-off-by: iosmanthus <[email protected]>

iosmanthus changed the title ~~introduce ApiV2~~ [WIP] Introduce API v2 May 12, 2022

Merge branch 'master' into api-v2

fcf959a

iosmanthus requested a review from pingyu May 12, 2022 09:49

pingyu reviewed May 12, 2022

View reviewed changes

iosmanthus added 9 commits May 15, 2022 23:50

git fire: refactoring the interface SplitRegion of CodecClientV2

5e1ea7f

Signed-off-by: iosmanthus <[email protected]>

go mod tidy

be0735e

Signed-off-by: iosmanthus <[email protected]>

fix split region issue in CodecClientV2

05f7be4

Signed-off-by: iosmanthus <[email protected]>

rearrange imports

74beddb

Signed-off-by: iosmanthus <[email protected]>

add version info for RegionCache

2d05771

Signed-off-by: iosmanthus <[email protected]>

delay request encoding to RPCClient

3e653ef

Signed-off-by: iosmanthus <[email protected]>

prevent multiple encoding

582656a

Signed-off-by: iosmanthus <[email protected]>

wip: debugging retry logic

e345f67

Signed-off-by: iosmanthus <[email protected]>

fix EpochNotMatch region decoding

4cde6c5

Signed-off-by: iosmanthus <[email protected]>

sunxiaoguang reviewed Jun 7, 2022

View reviewed changes

iosmanthus added 2 commits June 8, 2022 19:47

remove txn request branch

8b32847

Signed-off-by: iosmanthus <[email protected]>

copy request before encoding it

b987826

Signed-off-by: iosmanthus <[email protected]>

pingyu reviewed Jun 13, 2022

View reviewed changes

iosmanthus changed the title ~~[WIP] Introduce API v2~~ Introduce API v2 Jun 14, 2022

add gRPCDialOptions for rawkv.Client

a2d022b

Signed-off-by: iosmanthus <[email protected]>

pingyu reviewed Jun 14, 2022

View reviewed changes

fix api breaking change of raw.NewClient

8a826e6

Signed-off-by: iosmanthus <[email protected]>

pingyu reviewed Jun 14, 2022

View reviewed changes

iosmanthus requested review from disksing and sticnarf June 21, 2022 08:26

disksing reviewed Jun 21, 2022

View reviewed changes

address menglong's comments

4ac9b28

Signed-off-by: iosmanthus <[email protected]>

sticnarf reviewed Jun 22, 2022

View reviewed changes

internal/client/api_version.go Outdated Show resolved Hide resolved

internal/client/api_version.go Outdated Show resolved Hide resolved

haojinming reviewed Jul 1, 2022

View reviewed changes

tikvrpc/tikvrpc.go Show resolved Hide resolved

sunxiaoguang reviewed Jul 6, 2022

View reviewed changes

iosmanthus added 4 commits July 7, 2022 15:59

Merge branch 'master' of github.com:tikv/client-go into api-v2

e4e9a46

Signed-off-by: iosmanthus <[email protected]>

go mod tidy

6433126

Signed-off-by: iosmanthus <[email protected]>

address comments and fix linter

409e7d6

Signed-off-by: iosmanthus <[email protected]>

Merge branch 'master' of github.com:tikv/client-go into api-v2

589cb6a

Signed-off-by: iosmanthus <[email protected]>

fix api_test

e0d7958

Signed-off-by: iosmanthus <[email protected]>

sunxiaoguang reviewed Jul 7, 2022

View reviewed changes

iosmanthus added 3 commits July 7, 2022 23:21

fix TestScan

b10e445

Signed-off-by: iosmanthus <[email protected]>

fix TestScan

f19106c

Signed-off-by: iosmanthus <[email protected]>

fix compatibility

7fbd1df

Signed-off-by: iosmanthus <[email protected]>

sunxiaoguang approved these changes Jul 8, 2022

View reviewed changes

Merge branch 'master' into api-v2

e480276

sunxiaoguang merged commit 68cd1bc into tikv:master Jul 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce API v2 #494

Introduce API v2 #494

iosmanthus commented May 12, 2022 •

edited

Loading

pingyu May 12, 2022

pingyu May 12, 2022

pingyu May 12, 2022

pingyu May 12, 2022

sunxiaoguang Jun 7, 2022

sunxiaoguang Jun 7, 2022

iosmanthus Jun 8, 2022

sunxiaoguang Jun 7, 2022

pingyu Jun 13, 2022

pingyu Jun 14, 2022

iosmanthus Jun 14, 2022

pingyu Jun 14, 2022

iosmanthus Jun 14, 2022

pingyu left a comment

disksing Jun 21, 2022

disksing Jun 21, 2022

sunxiaoguang Jul 6, 2022

iosmanthus Jul 7, 2022

sunxiaoguang Jul 7, 2022

sunxiaoguang Jul 6, 2022

iosmanthus Jul 7, 2022 •

edited

Loading

iosmanthus commented Jul 7, 2022

sunxiaoguang Jul 7, 2022

sunxiaoguang left a comment

sunxiaoguang commented Jul 9, 2022

Introduce API v2 #494

Introduce API v2 #494

Conversation

iosmanthus commented May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pingyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iosmanthus Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

iosmanthus commented Jul 7, 2022

Choose a reason for hiding this comment

sunxiaoguang left a comment

Choose a reason for hiding this comment

sunxiaoguang commented Jul 9, 2022

iosmanthus commented May 12, 2022 •

edited

Loading

iosmanthus Jul 7, 2022 •

edited

Loading