[WIP] Add codec for encoding region for transaction client #162

georgeteo · 2020-09-13T00:33:57Z

In this github issue, it appears that transaction client does not encode/decode keys when querying PD.

This PR:

duplicates bytes encoder/decoder from tikv_utils
Adds a method to encode Key
Adds PdCodecClient that wraps/decorates PdClient encodes key and decodes start_key and end_key in region meta before delegating to underlying PdClient.
Swap the underlying PdClient in TransactionClient with the one that encodes/decodes keys

Reference Go client.

Caveat: I'm new to Rust so general Rust tips would be appreciated!

Signed-off-by: George Teo george.c.teo@gmail.com

georgeteo · 2020-09-13T05:35:17Z

src/pd/client.rs

+    pd: Arc<PdClient>
+}
+
+impl<PdC: PdClient + Send + Sync + 'static> PdClient for PdCodecClient<PdC> {


TODO: write tests

ekexium · 2020-09-14T02:12:42Z

You can check the commands and outputs of CI. Note you need to use latest nightly channel to build.
Seems to me the only error was an unresolved import and some format issues.

error[E0432]: unresolved import `crate::kv::codec::bytes`

   --> tikv-client-common/src/kv/codec.rs:134:9

    |

134 |     use crate::kv::codec::bytes;

Removing that line and cargo fmt should fix the build.

Also you need to sign-off your commit.

ekexium · 2020-09-14T02:13:15Z

PTAL @nrc @sticnarf

nrc

Thanks for the PR! The rest of it looks great.

nrc · 2020-09-14T04:31:26Z

src/pd/client.rs

@@ -266,6 +267,58 @@ impl<KvC: KvConnect + Send + Sync + 'static, Cl> PdRpcClient<KvC, Cl> {
    }
 }

+/// This client wraps a PdClient and encodes/decodes keys and regions according to
+/// TiKV transaction codec.
+pub struct PdCodecClient<PdClient = PdRpcClient> {


I don't think we need a separate client for this, you can just add the encoding to the existing client.

nrc · 2020-09-14T04:36:58Z

tikv-client-common/src/kv/codec.rs

+use std::{io::Write, ptr};
+
+const ENC_GROUP_SIZE: usize = 8;
+const ENC_MARKER: u8 = b'\xff';


Suggested change

const ENC_MARKER: u8 = b'\xff';

const ENC_MARKER: u8 = 0xff';

nrc · 2020-09-14T04:40:01Z

tikv-client-common/src/kv/key.rs

@@ -101,6 +102,14 @@ impl Key {
            Bound::Excluded(self)
        }
    }
+
+    #[inline]
+    pub fn as_encoded(&self) -> Key {


I think we should use the type system to differentiate between an encoded and unencoded key by adding an EncodedKey type

nrc · 2020-09-14T04:41:24Z

tikv-client-common/src/kv/key.rs

+    pub fn as_encoded(&self) -> Key {
+        let len = codec::max_encoded_bytes_size(self.0.len());
+        let mut encoded = Vec::with_capacity(len);
+        encoded.encode_bytes(self.into(), false).unwrap();


Suggested change

encoded.encode_bytes(self.into(), false).unwrap();

encoded.encode_bytes(self.0, false).unwrap();

georgeteo · 2020-09-14T05:16:39Z

@nrc: thanks for the quick review! Just to clarify the overarching point so I don't go down the wrong rabbit hole:

Have a single PdRpcClient
Handle RawKey and EncodedKey as two structs that impl a Key trait and wrap that concern in Key.
Use EncodedKey in TransactionClient and RawKey in RawClient?

Thoughts on naming EncodedKey as TransactionKey or TransactionalKey for TransactionClient to mirror RawKey <-> RawClient?

nrc · 2020-09-15T02:04:05Z

Handle RawKey and EncodedKey as two structs that impl a Key trait and wrap that concern in Key.

I don't think you need a Key trait - the idea is to prevent using an encoded key where a raw key is required and vice versa, a Key trait would circumvent that.

Use EncodedKey in TransactionClient and RawKey in RawClient?

AIUI, encoded keys are only used with PD and we use raw keys with TiKV.

Thoughts on naming EncodedKey as TransactionKey or TransactionalKey for TransactionClient to mirror RawKey <-> RawClient?

I don't think it makes sense, whether a key is encoded is orthogonal to which API it is used with.

ekexium · 2020-09-27T09:32:01Z

I made some changes:

Use a single PD client so that we don't need to write duplicated code. All encoding stuff is hiden in the client. Lower-level PD functions and higher-level clients do not care about encoding.
Fix a few bugs related to encoding
Find and fix a few bugs not related to encoding.
Construct a simple integration test to test with splitted regions.

ekexium

None and Some(empty) can both represent upper unbounded. I didn't comprehensively consider this change. Related to #174

src/pd/client.rs

ekexium · 2020-09-27T09:45:48Z

@sticnarf PTAL

tests/integration_tests.rs

sticnarf

LGTM

ekexium · 2020-09-28T10:13:15Z

Hi @georgeteo, we need you to sign off your commits to merge the PR. About the DCO check: https://github.com/probot/dco#how-it-works

Signed-off-by: George Teo <george.c.teo@gmail.com>

Signed-off-by: ekexium <ekexium@gmail.com>

Signed-off-by: longfangsong <longfangsong@icloud.com>

Signed-off-by: ekexium <ekexium@gmail.com>

…ehavior with TiKV Signed-off-by: ekexium <ekexium@gmail.com>

…rors Signed-off-by: ekexium <ekexium@gmail.com>

Signed-off-by: ekexium <ekexium@gmail.com>

Signed-off-by: George Teo <george.c.teo@gmail.com>

Signed-off-by: ekexium <ekexium@gmail.com>

georgeteo commented Sep 13, 2020

View reviewed changes

georgeteo mentioned this pull request Sep 13, 2020

How to debug InvalidReqRange error and tikv-ctl usage unclear? tikv/tikv#8626

Open

nrc reviewed Sep 14, 2020

View reviewed changes

ekexium mentioned this pull request Sep 21, 2020

Plan for releasing 0.1 version #171

Closed

15 tasks

ekexium force-pushed the fix-codec branch from 1e1bdc0 to fb35862 Compare September 23, 2020 10:59

ekexium reviewed Sep 27, 2020

View reviewed changes

src/pd/client.rs Show resolved Hide resolved

sticnarf reviewed Sep 28, 2020

View reviewed changes

tests/integration_tests.rs Outdated Show resolved Hide resolved

ekexium force-pushed the fix-codec branch 2 times, most recently from 07693f4 to 44c582c Compare September 28, 2020 09:30

sticnarf approved these changes Sep 28, 2020

View reviewed changes

georgeteo and others added 13 commits September 28, 2020 18:13

Add codec for encoding region for transaction client

586fbc4

Signed-off-by: George Teo <george.c.teo@gmail.com>

Fix build

f193811

Signed-off-by: George Teo <george.c.teo@gmail.com>

fix imports

1614e8e

Signed-off-by: George Teo <george.c.teo@gmail.com>

Decode start and end keys in region meta

b9c5735

Signed-off-by: George Teo <george.c.teo@gmail.com>

Fix CI build

10c2177

Signed-off-by: George Teo <george.c.teo@gmail.com>

optimize condition in stores_for_range

83f95a2

Signed-off-by: ekexium <ekexium@gmail.com>

update kvproto dependency

af23a9a

Signed-off-by: longfangsong <longfangsong@icloud.com>

bound region range by query range

8791b5f

Signed-off-by: ekexium <ekexium@gmail.com>

bound_range: improve code style

be4a127

Signed-off-by: ekexium <ekexium@gmail.com>

propagate error in map_region_to_store

871c5b8

Signed-off-by: ekexium <ekexium@gmail.com>

match on upper_bound

8ee145f

Signed-off-by: ekexium <ekexium@gmail.com>

handle None when converting BoundRange to KeyRange

976e051

Signed-off-by: ekexium <ekexium@gmail.com>

use unwrap_or_default instead of match

d8c2893

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium and others added 26 commits September 28, 2020 18:15

resolve comments: resume src/mock.rs, improve style

840e610

Signed-off-by: ekexium <ekexium@gmail.com>

resolve comments: improve style in mock-tikv

f44185e

Signed-off-by: ekexium <ekexium@gmail.com>

change semantics of raw_batch_delete; iterate once

5fb18f3

Signed-off-by: ekexium <ekexium@gmail.com>

resume proptest

f12595c

Signed-off-by: ekexium <ekexium@gmail.com>

mock tikv: distinguish between empty value and non-exsit keys; same b…

cff0042

…ehavior with TiKV Signed-off-by: ekexium <ekexium@gmail.com>

mock tikv: remove unnecessary clones; delete requests won't return er…

44fcc4b

…rors Signed-off-by: ekexium <ekexium@gmail.com>

mock tikv: update kvproto dependency

78d944d

Signed-off-by: ekexium <ekexium@gmail.com>

remove stale comments

785dfde

Signed-off-by: ekexium <ekexium@gmail.com>

txn/scan combines local buffer and fetched result

0ab5b67

Signed-off-by: ekexium <ekexium@gmail.com>

cache results of txn/scan

cc72fd5

Signed-off-by: ekexium <ekexium@gmail.com>

move key_only parameter from raw client to its scan methods

c239e8a

Signed-off-by: ekexium <ekexium@gmail.com>

rename txn.set to txn.put

0b6dc04

Signed-off-by: ekexium <ekexium@gmail.com>

move key_only from txn.scan to txn client

e26a79e

Signed-off-by: ekexium <ekexium@gmail.com>

raw bank transfer test

ba16c35

Signed-off-by: ekexium <ekexium@gmail.com>

integration tests: add raw client test

78a894c

Signed-off-by: ekexium <ekexium@gmail.com>

add serial txn bank transfer test

88c0b33

Signed-off-by: ekexium <ekexium@gmail.com>

fix integration tests

1869f99

Signed-off-by: ekexium <ekexium@gmail.com>

decode does nothing on empty key

fa48606

Signed-off-by: ekexium <ekexium@gmail.com>

use a single PD client, hide details in it; pass simple tests

5ff86ef

Signed-off-by: ekexium <ekexium@gmail.com>

use try_for_each

57b542a

Signed-off-by: ekexium <ekexium@gmail.com>

fix mock test

1aadaf1

Signed-off-by: ekexium <ekexium@gmail.com>

improve style, remove debug code

4b8091a

Signed-off-by: ekexium <ekexium@gmail.com>

check both key and value in test raw_req

437f5f0

Signed-off-by: ekexium <ekexium@gmail.com>

add some comments to PdClient to make the encoding part clear

ed58e7d

Signed-off-by: ekexium <ekexium@gmail.com>

Signing dco

75ae0b2

Signed-off-by: George Teo <george.c.teo@gmail.com>

Revert extra readme change

fed12b4

Signed-off-by: George Teo <george.c.teo@gmail.com>

georgeteo force-pushed the fix-codec branch from b709ed1 to fed12b4 Compare September 29, 2020 01:19

Merge branch 'master' into fix-codec

51597d4

Signed-off-by: ekexium <ekexium@gmail.com>

ekexium merged commit ad8ef07 into tikv:master Sep 29, 2020

ekexium mentioned this pull request Sep 29, 2020

Fill a missing situation in bound_range #174

Closed

	const ENC_MARKER: u8 = b'\xff';
	const ENC_MARKER: u8 = 0xff';

	encoded.encode_bytes(self.into(), false).unwrap();
	encoded.encode_bytes(self.0, false).unwrap();

[WIP] Add codec for encoding region for transaction client #162

[WIP] Add codec for encoding region for transaction client #162

Uh oh!

Conversation

georgeteo commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgeteo Sep 13, 2020

Choose a reason for hiding this comment

Uh oh!

ekexium commented Sep 14, 2020

Uh oh!

ekexium commented Sep 14, 2020

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

nrc Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

nrc Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

nrc Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

nrc Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

georgeteo commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrc commented Sep 15, 2020

Uh oh!

ekexium commented Sep 27, 2020

Uh oh!

ekexium left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekexium commented Sep 27, 2020

Uh oh!

Uh oh!

sticnarf left a comment

Choose a reason for hiding this comment

Uh oh!

ekexium commented Sep 28, 2020

Uh oh!

Uh oh!

georgeteo commented Sep 13, 2020 •

edited

Loading

georgeteo commented Sep 14, 2020 •

edited

Loading