Skip to content

storage: Add API V2 check for RawKV and TxnKV requests (part 2)#11228

Merged
ti-chi-bot merged 26 commits intotikv:masterfrom
pingyu:add_api_v2_check_part2
Dec 6, 2021
Merged

storage: Add API V2 check for RawKV and TxnKV requests (part 2)#11228
ti-chi-bot merged 26 commits intotikv:masterfrom
pingyu:add_api_v2_check_part2

Conversation

@pingyu
Copy link
Contributor

@pingyu pingyu commented Nov 3, 2021

What problem does this PR solve?

Issue Number: #10974

Problem Summary:

Add API V2 check for RawKV and TxnKV requests.

What is changed and how it works?

Proposal: TiKV API V2

What's Changed:

Add checker logics for the following API:

  • raw_batch_get_command
  • raw_batch_put
  • raw_delete
  • raw_delete_range
  • raw_batch_delete
  • raw_scan
  • raw_batch_scan
  • raw_get_key_ttl
  • raw_compare_and_swap_atomic
  • raw_batch_put_atomic
  • raw_batch_delete_atomic
  • raw_checksum
  • scan
  • scan_lock
  • delete_range

Related changes

No.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Breaking backward compatibility
    Breaking backward compatibility: See rfc TiKV API V2 for detail.

Release note 

None.

pingyu added 8 commits November 3, 2021 14:09
@pingyu
wip
09b15d3 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
wip
496eebc 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
wip
a70c75e 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
wip
4d68635 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
wip
1ef796e 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
wip
f953f88 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)
a9d4444 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)
98d7c82 
Signed-off-by: pingyu <yuping@pingcap.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 3, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • andylokandy
  • zhongzc

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.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 3, 2021
@pingyu
Copy link
Contributor Author

pingyu commented Nov 3, 2021

/cc @andylokandy @iosmanthus

iosmanthus
iosmanthus reviewed Nov 4, 2021
View reviewed changes
andylokandy
andylokandy reviewed Nov 4, 2021
View reviewed changes
src/storage/mod.rs
Comment on lines +940 to +941
raw_start_key: Option<Vec<u8>>,
raw_end_key: Option<Vec<u8>>,
Copy link
Contributor

@andylokandy andylokandy Nov 4, 2021

Choose a reason for hiding this comment

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

We can accept Vec<u8> and call Key::from_raw_maybe_unbounded here.

Copy link
Contributor Author

@pingyu pingyu Nov 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I check unbounded and transform to Option<Vec<u8>> in service/kv.rs. So just Key::from_raw is needed here.

Copy link
Contributor

@andylokandy andylokandy Nov 10, 2021

Choose a reason for hiding this comment

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

I mean by checking it here, we save repeating the logic of Key::from_raw_maybe_unbounded in kv.rs and also no long bother to raw_start_key.as_ref().map(|x| Key::from_raw(&x[..])) later on.

Copy link
Contributor Author

@pingyu pingyu Nov 12, 2021

Choose a reason for hiding this comment

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

But if so, we need to check is_empty() again before passing to check_api_version().

Besides, the signature as Option telling that the argument is optional for unbounded. I think it's more readable.

src/storage/mod.rs
start_key: Key,
end_key: Option<Key>,
raw_start_key: Vec<u8>,
raw_end_key: Option<Vec<u8>>,
Copy link
Contributor

@andylokandy andylokandy Nov 4, 2021

Choose a reason for hiding this comment

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

ditto

andylokandy
andylokandy reviewed Nov 10, 2021
View reviewed changes
andylokandy
andylokandy reviewed Nov 10, 2021
View reviewed changes
@pingyu
Copy link
Contributor Author

pingyu commented Nov 12, 2021

@andylokandy
All comments are addressed or replied. PTAL, thanks~

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2021
@pingyu
Merge remote-tracking branch 'upstream/master' into add_api_v2_check_…
97774e6 
…part2

Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
resolve conflict
293ac10 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
add raw_key_maybe_unbounded_into_option
719f604 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)
540a6b7 
Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)
36ca2c7 
Signed-off-by: pingyu <yuping@pingcap.com>
andylokandy
andylokandy reviewed Dec 1, 2021
View reviewed changes
pingyu and others added 3 commits December 2, 2021 11:21
@pingyu
Merge remote-tracking branch 'upstream/master' into add_api_v2_check_…
e9e0990 
…part2

Signed-off-by: pingyu <yuping@pingcap.com>
@pingyu
ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)
a7201ef 
Signed-off-by: pingyu <yuping@pingcap.com>
@andylokandy
remove make_invalid_key_prefix_err
fa8c297 
Signed-off-by: andylokandy <andylokandy@hotmail.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 3, 2021
@andylokandy andylokandy requested a review from zhongzc December 3, 2021 10:32
@ti-chi-bot 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 Dec 3, 2021
@andylokandy
Copy link
Contributor

andylokandy commented Dec 3, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 3, 2021

@andylokandy: 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
Copy link
Member

ti-chi-bot commented Dec 3, 2021

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

DetailsCommit hash: 4a67bc95a4a4cb29d1a57a9da7ae6ceb9c89ec0e

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 3, 2021
@pingyu
Copy link
Contributor Author

pingyu commented Dec 3, 2021

/run-all-tests

1 similar comment
@pingyu
Copy link
Contributor Author

pingyu commented Dec 4, 2021

/run-all-tests

@pingyu pingyu force-pushed the add_api_v2_check_part2 branch from 630010a to fa8c297 Compare December 6, 2021 04:03
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Dec 6, 2021
@andylokandy
Copy link
Contributor

andylokandy commented Dec 6, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 6, 2021

@andylokandy: 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
Copy link
Member

ti-chi-bot commented Dec 6, 2021

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

DetailsCommit hash: de4e6ef

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 6, 2021
@ti-chi-bot ti-chi-bot merged commit 0cfe9f7 into tikv:master Dec 6, 2021
@pingyu pingyu mentioned this pull request Dec 6, 2021
Merged
ethercflow pushed a commit to ethercflow/tikv that referenced this pull request Dec 7, 2021
@pingyu @andylokandy @ethercflow
storage: Add API V2 check for RawKV and TxnKV requests (part 2) (tikv…
351fb4d 
…#11228)

* wip

Signed-off-by: pingyu <yuping@pingcap.com>

* wip

Signed-off-by: pingyu <yuping@pingcap.com>

* wip

Signed-off-by: pingyu <yuping@pingcap.com>

* wip

Signed-off-by: pingyu <yuping@pingcap.com>

* wip

Signed-off-by: pingyu <yuping@pingcap.com>

* wip

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* resolve conflict

Signed-off-by: pingyu <yuping@pingcap.com>

* add raw_key_maybe_unbounded_into_option

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: address comments

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* ref tikv#10974: Add API V2 check for RawKV and TxnKV requests (part 2)

Signed-off-by: pingyu <yuping@pingcap.com>

* remove make_invalid_key_prefix_err

Signed-off-by: andylokandy <andylokandy@hotmail.com>

Co-authored-by: andylokandy <andylokandy@hotmail.com>
@BusyJay
Copy link
Member

BusyJay commented Dec 8, 2021

This PR makes test test_txn_store_rawkv unstable in recent builds. See https://ci.pingcap.net/blue/organizations/jenkins/tikv_ghpr_test/detail/tikv_ghpr_test/12889/pipeline/115.

@pingyu
Copy link
Contributor Author

pingyu commented Dec 8, 2021

This PR makes test test_txn_store_rawkv unstable in recent builds. See https://ci.pingcap.net/blue/organizations/jenkins/tikv_ghpr_test/detail/tikv_ghpr_test/12889/pipeline/115.

Let me see see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iosmanthus iosmanthus iosmanthus left review comments

@andylokandy andylokandy andylokandy approved these changes

@zhongzc zhongzc zhongzc approved these changes

Assignees

No one assigned

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pingyu @ti-chi-bot @andylokandy @BusyJay @iosmanthus @zhongzc