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

add scan_messages for iterating over messages #234

Merged
merged 11 commits into from
Jul 14, 2022
Merged

Conversation

5kbpers
Copy link
Member

@5kbpers 5kbpers commented Jun 27, 2022

Signed-off-by: 5kbpers [email protected]

@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #234 (c56d519) into master (be26141) will decrease coverage by 0.44%.
The diff coverage is 69.23%.

❗ Current head c56d519 differs from pull request most recent head 90ce797. Consider uploading reports for the commit 90ce797 to get more accurate results

@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
- Coverage   97.19%   96.74%   -0.45%     
==========================================
  Files          30       30              
  Lines        9368     9517     +149     
==========================================
+ Hits         9105     9207     +102     
- Misses        263      310      +47     
Impacted Files Coverage Δ
src/memtable.rs 97.49% <67.70%> (-1.69%) ⬇️
src/engine.rs 96.30% <71.23%> (-1.28%) ⬇️
src/log_batch.rs 97.04% <0.00%> (+0.43%) ⬆️
src/env/default.rs 92.69% <0.00%> (+0.56%) ⬆️

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 be26141...90ce797. Read the comment docs.

Signed-off-by: 5kbpers <[email protected]>
@5kbpers 5kbpers changed the title add interface that returns the last value that key is equal to or less than given key add scan_message for iterating over messages Jun 27, 2022
src/engine.rs Outdated
) -> Result<()>
where
S: Message,
C: FnMut(&[u8], S) -> Result<bool>,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think user should use crate::Result.

Suggested change
C: FnMut(&[u8], S) -> Result<bool>,
C: FnMut(&[u8], S) -> bool,

Copy link
Member Author

Choose a reason for hiding this comment

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

Used for catching errors when something goes wrong on value scanned.

Copy link
Member

Choose a reason for hiding this comment

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

The point is user shouldn't fake their own errors as crate::Error. Say you can get away with wrapping it into a Error::Other now, but what if the Other variant is removed in the future?

assert_eq!(memtable.get_leq(b"key9"), Some(v5.to_vec()));

let mut res = vec![];
memtable
Copy link
Member

Choose a reason for hiding this comment

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

Move some tests to engine.rs to improve coverage

src/engine.rs Outdated
Comment on lines 230 to 231
start_key: &[u8],
end_key: &[u8],
Copy link
Member

Choose a reason for hiding this comment

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

Option?

5kbpers added 5 commits June 27, 2022 20:38
Signed-off-by: 5kbpers <[email protected]>
Signed-off-by: 5kbpers <[email protected]>
Signed-off-by: 5kbpers <[email protected]>
Signed-off-by: 5kbpers <[email protected]>
Signed-off-by: 5kbpers <[email protected]>
Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

Rest looks good.

src/engine.rs Outdated
Ok(())
}

pub fn scan<C>(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn scan<C>(
pub fn scan_raw_messages<C>(

src/engine.rs Outdated
@@ -224,6 +224,47 @@ where
Ok(None)
}

pub fn scan_message<S, C>(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn scan_message<S, C>(
pub fn scan_messages<S, C>(

Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

Oh, update CHANGELOG.md.

@tabokie tabokie changed the title add scan_message for iterating over messages add scan_messages for iterating over messages Jul 14, 2022
Signed-off-by: 5kbpers <[email protected]>
CHANGELOG.md Outdated
@@ -9,6 +9,7 @@
### New Features

* Add `PerfContext` which records detailed time breakdown of the write process to thread-local storage.
* Add `Engine::scan_messages` and `Engine::scan_raw_messages` for iterating over written key-values.
Copy link
Member

Choose a reason for hiding this comment

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

Better under "Public API Changes"

Signed-off-by: 5kbpers <[email protected]>
@tabokie tabokie merged commit f3c268b into tikv:master Jul 14, 2022
@5kbpers 5kbpers deleted the get_leq branch July 14, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants