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

tikvclient: Add endKey param to Scanner #8178

Merged
merged 19 commits into from
Nov 8, 2018

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented Nov 5, 2018

What problem does this PR solve?

This PR is extracted from #8081 .

What is changed and how it works?

This PR updated kvproto to the newest so there is the EndKey param in kv scan RPC call. Then endKey was added to Scanner as an extra param of func newScanner.

Also, an upperBound parameter was added to the Seek method of Retriever interface, and the method was renamed to Iter.

This PR should be merged after tikv/tikv#3720 . (Done)

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has interface methods change
    • Seek and SeekReverse of Retriever are renamed to Iter and IterReverse

Side effects

None

Related changes

None

MyonKeminta and others added 9 commits October 28, 2018 17:36
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
@MyonKeminta MyonKeminta changed the title tikvclient: Add endKey param to Scanner [DNM] tikvclient: Add endKey param to Scanner Nov 5, 2018
Signed-off-by: MyonKeminta <[email protected]>
@MyonKeminta
Copy link
Contributor Author

/run-integration-tests tikv=pr/3720

@MyonKeminta MyonKeminta changed the title [DNM] tikvclient: Add endKey param to Scanner tikvclient: Add endKey param to Scanner Nov 6, 2018
@MyonKeminta
Copy link
Contributor Author

/run-integration-tests

@MyonKeminta
Copy link
Contributor Author

@disksing @zhangjinpeng1987 PTAL

@disksing
Copy link
Contributor

disksing commented Nov 6, 2018

LGTM. PTAL @zhangjinpeng1987

@MyonKeminta
Copy link
Contributor Author

Emmmm why is circleci canceled..

@MyonKeminta
Copy link
Contributor Author

@tiancaiamao PTAL..

@@ -199,7 +202,7 @@ func (s *Scanner) getData(bo *Backoffer) error {
// No more data in current Region. Next getData() starts
// from current Region's endKey.
s.nextStartKey = loc.EndKey
if len(loc.EndKey) == 0 {
if len(loc.EndKey) == 0 || (len(s.endKey) > 0 && kv.Key(s.nextStartKey).Cmp(kv.Key(s.endKey)) >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find s.endKey == nil and s.endKey == []byte{} is not distinguished
Should we make it clear? or it's deliberately?

s.endKey == nil -- nobody set it
s.endKey == []byte{} -- it's actually set to a wrong value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tries to regards nil and empty the same, so I simply use len to judge whether it's set.

@tiancaiamao
Copy link
Contributor

/run-all-tests

@MyonKeminta
Copy link
Contributor Author

/rebuild

@MyonKeminta
Copy link
Contributor Author

@tiancaiamao Can you help me re-run the circleci?

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added status/all tests passed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 8, 2018
@tiancaiamao tiancaiamao merged commit d125958 into pingcap:master Nov 8, 2018
@MyonKeminta MyonKeminta deleted the misono/ticlient-scan-end-key branch November 9, 2018 04:07
MyonKeminta added a commit to MyonKeminta/tidb that referenced this pull request Nov 9, 2018
MyonKeminta added a commit to MyonKeminta/tidb that referenced this pull request Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants