Skip to content

Conversation

@skyzh
Copy link
Contributor

@skyzh skyzh commented Feb 10, 2022

Signed-off-by: Alex Chi [email protected]

What's changed and what's your intention?

In the original implementation, if we have:

a, ab, bc

and it occurs that the last key we get is a. next_key(a) = b. We will miss ab.

This PR fixes the bug.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

#62

@skyzh skyzh requested review from hzxa21 and zwang28 February 10, 2022 02:53
@github-actions github-actions bot added the type/fix Type: Bug fix. Only for pull requests. label Feb 10, 2022
Copy link
Contributor

@TennyZhuang TennyZhuang left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Alex Chi <[email protected]>
Signed-off-by: Alex Chi <[email protected]>
.scan_with_start_key(last_key.to_vec(), Some(limit), self.epoch)
.await?;
assert!(!self.buf.is_empty());
assert_eq!(self.buf.first().as_ref().unwrap().0, last_key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(self.buf.first().as_ref().unwrap().0, last_key);
assert_eq!(buf.first().as_ref().unwrap().0, last_key);

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #184 (a8423d8) into main (ff6d11b) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #184      +/-   ##
============================================
- Coverage     74.49%   74.49%   -0.01%     
  Complexity     2654     2654              
============================================
  Files           837      837              
  Lines         47761    47763       +2     
  Branches       1562     1562              
============================================
+ Hits          35581    35582       +1     
- Misses        11381    11382       +1     
  Partials        799      799              
Flag Coverage Δ
java 62.08% <ø> (ø)
rust 79.86% <88.88%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/storage/src/table/mview.rs 71.05% <88.88%> (+0.51%) ⬆️
rust/storage/src/hummock/version_manager.rs 39.05% <0.00%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6d11b...a8423d8. Read the comment docs.

@skyzh skyzh merged commit 956ba00 into main Feb 10, 2022
@skyzh skyzh deleted the skyzh/fix-missing-key branch February 10, 2022 06:04
@BugenZhao BugenZhao mentioned this pull request Feb 10, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/fix Type: Bug fix. Only for pull requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants