Reserve key space id encoding in backup convert by haojinming · Pull Request #12759 · tikv/tikv

haojinming · 2022-06-06T03:22:52Z

Signed-off-by: haojinming jinming.hao@pingcap.com

What is changed and how it works?

Issue Number: Close #12758

What's Changed:

Add encoding of key space id in apiv2 key when backup convert.

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test

Side effects

Release note


Add key space id encoding in apiv2 key when backup convert.

ti-chi-bot · 2022-06-06T03:22:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

pingyu
sunxiaoguang

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: haojinming <jinming.hao@pingcap.com>

pingyu · 2022-06-06T05:22:26Z

components/api_version/src/api_v1.rs

-                let (mut user_key, _) = ApiV2::decode_raw_key(&Key::from_encoded_slice(key), true)?;
-                user_key.remove(0); // remove first byte `RAW_KEY_PREFIX`
-                Ok(Self::encode_raw_key_owned(user_key, None))
+                Err(EngineError::Other(box_err!("unsupported conversion")))


Suggested change

Err(EngineError::Other(box_err!("unsupported conversion")))

Err(box_err!("unsupported conversion"))

The same to other three similar.

Maybe just panic ? The conversion from V2 to V1/V1TTL should be rejected by TiKV-BR, and when receive backup request. Other than the moment to convert key version.

done, mark apiv2->apiv1 as unreachable

pingyu · 2022-06-06T05:24:10Z

components/api_version/src/api_v2.rs

+        let mut apiv2_key = Vec::with_capacity(ApiV2::get_encode_len(key.len() + 2));
+        apiv2_key.push(RAW_KEY_PREFIX);
+        let mut tmp_buf = [0; MAX_VARINT64_LENGTH];
+        let written = NumberCodec::encode_var_u64(&mut tmp_buf, DEFAULT_KEY_SPACE_ID);


Suggest to try making written be constant. Or just write a 0 and verify it's always equal to var_u64 in unit test.

Let's make key space fixed.

pingyu · 2022-06-06T05:25:40Z

components/api_version/src/api_v2.rs

    }

+    pub fn add_prefix(key: &[u8]) -> Vec<u8> {
+        let mut apiv2_key = Vec::with_capacity(ApiV2::get_encode_len(key.len() + 2));


Reserve capacity after encoded keyspace id is got and avoid hard coding the length.

pingyu · 2022-06-06T05:34:13Z

components/backup/src/endpoint.rs

                        // the file range need be encoded as v2 format.
                        // And range in response keep in v1 format.
-                        let (start, end) = codec.convert_key_range_to_dst_version(
+                        let ret = codec.convert_key_range_to_dst_version(


Reject V2 -> V1/V1TTL earlier, e.g. KeyValueCodec::check_backup_api_version.

Signed-off-by: haojinming <jinming.hao@pingcap.com>

haojinming · 2022-06-06T09:20:55Z

/run-build

haojinming · 2022-06-06T09:21:04Z

/run-test

overvenus · 2022-06-06T09:49:33Z

components/api_version/src/api_v1.rs

-                user_key.remove(0); // remove first byte `RAW_KEY_PREFIX`
-                Ok(Self::encode_raw_key_owned(user_key, None))
-            }
+            ApiVersion::V2 => unreachable!(), // reject apiv2 -> apiv1 conversion


It will panic TiKV if the arm matches. What about returning an error?

backup process do check the apiversion in

tikv/components/backup/src/utils.rs

Lines 127 to 133 in ba391ff

pub fn check_backup_api_version(&self, start_key: &[u8], end_key: &[u8]) -> bool {

if self.is_raw_kv

&& self.cur_api_ver != self.dst_api_ver

&& self.dst_api_ver != ApiVersion::V2

{

return false;

}

to ensure that this branch is unreachable. Handle error will make backup process a little complicated here

tikv/components/backup/src/endpoint.rs

Lines 213 to 216 in ba391ff

let (start, end) = codec.convert_key_range_to_dst_version(

msg.start_key.clone(),

msg.end_key.clone(),

);

. And we want take this pr in v6.1.0 release, so we need make min changes.

Agree should return error instead of just panicking.

@overvenus @BusyJay error handling is added, PTAL~ thanks.

iosmanthus · 2022-06-06T09:55:05Z

components/api_version/src/api_v2.rs

-                start_key.insert(0, RAW_KEY_PREFIX);
+                start_key = ApiV2::add_prefix(&start_key);
                if end_key.is_empty() {
                    end_key.insert(0, RAW_KEY_PREFIX_END);


The EMPTY end key should be mapped to r001?.

Another question, when do we need this function while executing backup and restore.

This is a conversion scenario, no key space exists in apiv1, so I think we need set 'r' + 1 as the whole end. Anyway, 'r' + 1 and r001 both works here. @pingyu How do you think?

This function is needed when return backup response, range in BackupResponse.File need to be the apiv2 format if convert from apiv1/v1ttl to apiv2.

En... I suggest r001 . It is a reasonble range start & end for default key space. Although both are ok here.

After consideration, i think use r001 as the end is better. r001 as end can avoid some useless download and ingest during restore if target tikv cluster is not empty. I have changed the end as r001. PTAL~ @iosmanthus @pingyu

Signed-off-by: haojinming <jinming.hao@pingcap.com>

pingyu

LGTM~

Signed-off-by: haojinming <jinming.hao@pingcap.com>

BusyJay · 2022-06-07T03:15:50Z

components/api_version/src/api_v2.rs

 pub const TXN_KEY_PREFIX: u8 = b'x';
 pub const TIDB_META_KEY_PREFIX: u8 = b'm';
 pub const TIDB_TABLE_KEY_PREFIX: u8 = b't';
+pub const DEFAULT_KEY_SPACE_ID: [u8; 3] = [0, 0, 0]; // reserve 3 bytes for key space id.


Why? I thought it should be just one byte?

We plan to use 3 bytes fixed length to encode key space id, support more than 16 million key space. And we does not use var int encoding incase there are some issues we may miss.

Connor1996

LGTM

iosmanthus · 2022-06-07T06:26:10Z

/merge

ti-chi-bot · 2022-06-07T06:26:11Z

@iosmanthus: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-06-07T06:26:13Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 16796b8

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2022-06-07T06:34:57Z

cherry pick to release-6.1 in PR #12776

close #12758, ref #12759 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: haojinming <jinming.hao@pingcap.com>

) close tikv#12758, ref tikv#12759 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: haojinming <jinming.hao@pingcap.com> Signed-off-by: joccau <zak.zhao@pingcap.com>

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2022

add key space encoding in apiv2 key

3154629

Signed-off-by: haojinming <jinming.hao@pingcap.com>

haojinming force-pushed the br_convert_new branch from 6e2a944 to 3154629 Compare June 6, 2022 03:33

return error for downgrade backup

7460ac2

Signed-off-by: haojinming <jinming.hao@pingcap.com>

pingyu reviewed Jun 6, 2022

View reviewed changes

mark apiv2->apiv1 unreachable

ce49978

Signed-off-by: haojinming <jinming.hao@pingcap.com>

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/needs-triage-completed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2022

haojinming added 2 commits June 6, 2022 14:22

fix review comments

809e055

Signed-off-by: haojinming <jinming.hao@pingcap.com>

hard code 0 as vr u64 encoded skey space id

0115e45

Signed-off-by: haojinming <jinming.hao@pingcap.com>

haojinming changed the title ~~Add key space id encoding in backup convert~~ Reserve key space id encoding in backup convert Jun 6, 2022

encode key space is with 3 bytes

1847674

Signed-off-by: haojinming <jinming.hao@pingcap.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2022

sunxiaoguang approved these changes Jun 6, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 6, 2022

Merge branch 'master' into br_convert_new

edcce0b

overvenus reviewed Jun 6, 2022

View reviewed changes

iosmanthus reviewed Jun 6, 2022

View reviewed changes

haojinming added 4 commits June 6, 2022 18:34

fix backup unit test

14bbeb4

Signed-off-by: haojinming <jinming.hao@pingcap.com>

fix integration test

e184a17

Signed-off-by: haojinming <jinming.hao@pingcap.com>

use r001 as the backyo file end

50002f2

Signed-off-by: haojinming <jinming.hao@pingcap.com>

fix review comments

4e7f415

Signed-off-by: haojinming <jinming.hao@pingcap.com>

pingyu approved these changes Jun 6, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 6, 2022

haojinming added 3 commits June 7, 2022 10:20

error handling in conversion

a117611

Signed-off-by: haojinming <jinming.hao@pingcap.com>

improve error msg info

6bfc72e

Signed-off-by: haojinming <jinming.hao@pingcap.com>

make error handling more clear

16796b8

Signed-off-by: haojinming <jinming.hao@pingcap.com>

BusyJay reviewed Jun 7, 2022

View reviewed changes

pingyu added the needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. label Jun 7, 2022

Connor1996 approved these changes Jun 7, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 7, 2022

ti-chi-bot merged commit 62545b0 into tikv:master Jun 7, 2022

ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Jun 7, 2022

cherry pick tikv#12759 to release-6.1

9474599

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jun 7, 2022

Reserve key space id encoding in backup convert (#12759) #12776

Merged

haojinming deleted the br_convert_new branch June 7, 2022 06:54

ti-chi-bot pushed a commit that referenced this pull request Jun 7, 2022

Reserve key space id encoding in backup convert (#12759) (#12776)

c26134b

close #12758, ref #12759 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: haojinming <jinming.hao@pingcap.com>

	Err(EngineError::Other(box_err!("unsupported conversion")))
	Err(box_err!("unsupported conversion"))

	pub fn check_backup_api_version(&self, start_key: &[u8], end_key: &[u8]) -> bool {
	if self.is_raw_kv
	&& self.cur_api_ver != self.dst_api_ver
	&& self.dst_api_ver != ApiVersion::V2
	{
	return false;
	}

	let (start, end) = codec.convert_key_range_to_dst_version(
	msg.start_key.clone(),
	msg.end_key.clone(),
	);

Conversation

haojinming commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haojinming commented Jun 6, 2022

Uh oh!

haojinming commented Jun 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iosmanthus Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haojinming Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haojinming Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pingyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

iosmanthus commented Jun 7, 2022

Uh oh!

ti-chi-bot commented Jun 7, 2022

Uh oh!

ti-chi-bot commented Jun 7, 2022

Uh oh!

ti-srebot commented Jun 7, 2022

Uh oh!

Reviewers

haojinming commented Jun 6, 2022 •

edited

Loading

ti-chi-bot commented Jun 6, 2022 •

edited

Loading

iosmanthus Jun 6, 2022 •

edited

Loading

haojinming Jun 6, 2022 •

edited

Loading

haojinming Jun 6, 2022 •

edited

Loading