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

storage: support reverse raw_scan and raw_batch_scan #3724

Merged
merged 27 commits into from Nov 15, 2018

Conversation

@kamijin-fanta
Copy link
Contributor

commented Oct 30, 2018

What have you changed? (mandatory)

What are the type of the changes? (mandatory)

  • New feature (non-breaking change which adds functionality)

How has this PR been tested? (mandatory)

yes. I added unit test for raw_batch_scan.

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no.

Does this PR affect tidb-ansible update? (mandatory)

no.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Oct 30, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

Great job!
Please sign-off your commits and run cargo fmt

@kamijin-fanta kamijin-fanta force-pushed the kamijin-fanta:master branch from a3c5114 to d1ab390 Oct 30, 2018
@kamijin-fanta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

thanks!
I used "sign-off" for the first time. is this ok?

@kamijin-fanta kamijin-fanta force-pushed the kamijin-fanta:master branch from d1ab390 to 08aea28 Oct 30, 2018
Signed-off-by: kamijin_fanta <kamijin@live.jp>
- for support reverse option pingcap/kvproto#304

Signed-off-by: kamijin_fanta <kamijin@live.jp>
- add reverse request test cases
- support reverse in check_key_range
- for support reverse option pingcap/kvproto#304

Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta kamijin-fanta force-pushed the kamijin-fanta:master branch from 08aea28 to 2e0ff00 Oct 30, 2018
@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

Yes! And, please also fix the error messages from CircleCI

@MyonKeminta MyonKeminta changed the title support reverse scan support reverse raw_scan Oct 30, 2018
Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta kamijin-fanta force-pushed the kamijin-fanta:master branch from 2e0ff00 to a99f94f Oct 30, 2018
@MyonKeminta MyonKeminta changed the title support reverse raw_scan support reverse raw_scan and raw_batch_scan Oct 30, 2018
src/storage/mod.rs Outdated Show resolved Hide resolved
src/storage/mod.rs Show resolved Hide resolved
@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

In fact, I think it's better if the range to scan is always [start_key, end_key) whenever we are scanning forward or backward, which is more close to our semantics of the term "start_key" and "end_key".
@breeswish @zhangjinpeng1987 How do you think about it?

Signed-off-by: kamijin_fanta <kamijin@live.jp>
Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

lowerBound / upperBound and startKey / endKey are distinguished in the code.
so, I wrote such a code.

@breeswish

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Actually currently our semantic is that there is a range [lower_bound, upper_bound). when we do forward scan, we scan from lower_bound to upper_bound in the range. When we do reverse scan, we scan from upper_bound to lower_bound in the range. Maybe we can rename start_key to lower_bound and end_key to upper_bound to be less ambiguous?

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

@breeswish Seems it's not easy, since you may need to change the fields' name in kvproto

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

@breeswish oh, sorry, it seems renaming fields in kvproto won't break compatibility between tikv and rpc client..?

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

At least I think the fields of KeyRange should keep their meanings.
Neither should we rename the fields in it.

In coprocessor.proto:

// [start, end)
message KeyRange {
    bytes start = 1;
    bytes end = 2;
}

In kvrpcpb.proto

message KeyRange {
  bytes start_key = 1;
  bytes end_key = 2;
}

If we change them, we may introduce too much changes to TiKV and TiDB.
I think it's better to let [start_key, end_key) equivalent to [lower_bound, upper_bound) now.

@kamijin-fanta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

the consistency between startKey of ScanRequest and startKey of inside is lost.

message ScanRequest {
    Context context = 1;
    bytes start_key = 2;
    uint32 limit = 3;
    uint64 version = 4;
    bool key_only = 5;
    bool reverse = 6;
}

https://github.com/pingcap/kvproto/blob/855d2192cdc79c7353cf36e6b60e049bf436dc8a/proto/kvrpcpb.proto#L94-L101

I think it is easy to understand clearly distinguish between startKey and lowerBound.

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

OK. Let's accept this. But please add some comments to explain why start_key is the greater one.

src/storage/mod.rs Outdated Show resolved Hide resolved
- check_key_ranges
- no specify end_key in reverse batch scan request

Signed-off-by: kamijin_fanta <kamijin@live.jp>
@kamijin-fanta kamijin-fanta force-pushed the kamijin-fanta:master branch from 5ef162b to ede459d Nov 7, 2018
- invert reverse option in raw_scan, reverse_raw_scan

Signed-off-by: kamijin_fanta <kamijin@live.jp>
@breeswish

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

For the following two ranges, it looks wired in the reverse mode.. The first one is a, b, c and the second one is c, b, a. I prefer to keep the same global total order for all possible ranges in reverse mode, i.e. for every range (a_i, b_i) at index i, we have a_i > a_i+1.

        let ranges = make_ranges(vec![
            (b"a3".to_vec(), b"a".to_vec()),
            (b"b3".to_vec(), b"b".to_vec()),
            (b"c3".to_vec(), b"c".to_vec()),
        ]);
         let ranges = make_ranges(vec![
            (b"c3".to_vec(), vec![]),
            (b"b3".to_vec(), vec![]),
            (b"a3".to_vec(), vec![]),
        ]);
@breeswish

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

How do you think @MyonKeminta

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

@breeswish IMO, I think both are ok..

Copy link
Contributor

left a comment

LGTM

@MyonKeminta MyonKeminta changed the title support reverse raw_scan and raw_batch_scan storage: support reverse raw_scan and raw_batch_scan Nov 9, 2018
@siddontang

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2018

PTAL @breeswish

@breeswish

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

Is it possible to specify ranges in reverse mode like below?

        let ranges = make_ranges(vec![
            (b"c3".to_vec(), b"c".to_vec()),
            (b"b3".to_vec(), b"b".to_vec()),
            (b"a3".to_vec(), b"a".to_vec()),
        ]);
@kamijin-fanta

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

Copy link
Member

left a comment

Okay

@breeswish breeswish added the S: LGT2 label Nov 13, 2018
@kamijin-fanta

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

what is the status of this PR?

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Oh, sorry
/run-integration-tests

@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

/run-integration-common-test

1 similar comment
@MyonKeminta

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

/run-integration-common-test

@MyonKeminta MyonKeminta merged commit 3025141 into tikv:master Nov 15, 2018
2 of 3 checks passed
2 of 3 checks passed
ci/circleci: test CircleCI is running your tests
Details
DCO All commits are signed off!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
@kamijin-fanta

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

Thank you for the merge! 🎉

MyonKeminta added a commit to MyonKeminta/tikv that referenced this pull request Jul 29, 2019
MyonKeminta added a commit to MyonKeminta/tikv that referenced this pull request Jul 29, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
youjiali1995 added a commit that referenced this pull request Aug 2, 2019
* storage: Support reverse raw_scan and raw_batch_scan (#3724)

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

* Update kvproto

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.