-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 size properties collector #2097
Conversation
556542c
to
0e9c6bc
Compare
876e609
to
3f492ac
Compare
@@ -30,6 +32,9 @@ const PROP_NUM_PUTS: &'static str = "tikv.num_puts"; | |||
const PROP_NUM_VERSIONS: &'static str = "tikv.num_versions"; | |||
const PROP_MAX_ROW_VERSIONS: &'static str = "tikv.max_row_versions"; | |||
const PROP_NUM_ERRORS: &'static str = "tikv.num_errors"; | |||
const PROP_TOTAL_SIZE: &'static str = "tikv.total_size"; | |||
const PROP_SIZE_INDEX: &'static str = "tikv.size_index"; | |||
const PROP_SIZE_INDEX_DISTANCE: u64 = 4 * 1024 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about if the SST target file size is smaller than 4 MB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we collect only the start key and the end key.
@@ -183,6 +283,18 @@ impl UserProperties { | |||
buf.encode_u64(value).unwrap(); | |||
self.encode(name, buf); | |||
} | |||
|
|||
// Format: | klen | k | v.size | v.offset | | |||
pub fn encode_handles(&mut self, name: &str, handles: &BTreeMap<Vec<u8>, IndexHandle>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not belongs to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because IndexHandle
is a concept of SizeProperties
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, it can be a common struct in the future. For example, we may add handles for rows count later.
@@ -167,6 +172,101 @@ impl TablePropertiesCollectorFactory for MvccPropertiesCollectorFactory { | |||
} | |||
} | |||
|
|||
#[derive(Clone, Debug, Default)] | |||
pub struct IndexHandle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mentioned, how to add rows informations in this struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, we may have a RowsProperties
like:
struct RowsProperties {
total_rows: u64,
index_handles: BTreeMap<Vec<u8>, IndexHandle>,
}
It needs to encode the index_handles
too, so I put the encode_handles()
in a common place.
Besides, I prefer to put those encode/decode in one place.
src/util/properties.rs
Outdated
while !buf.is_empty() { | ||
let klen = try!(buf.decode_u64()); | ||
let mut k = vec![0; klen as usize]; | ||
try!(buf.read_exact(k.as_mut_slice())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use &mut k
let mut range = self.index_handles.range::<[u8], _>((Included(start), Unbounded)); | ||
let start_offset = match range.next() { | ||
Some((_, v)) => v.offset, | ||
None => return 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any test to cover None here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see test cases here.
e5ea9a8
to
d997313
Compare
LGTM |
PTAL @zhangjinpeng1987 |
LGTM |
d997313
to
43ba847
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Collect size properties from CF_DEFAULT and CF_WRITE.
Don't merge until #2096 has been merged.