Support db with ttl open #227
Conversation
|
@fredchenbj Thank you for the PR, I will take a look ASAP. #225 |
|
@fredchenbj CI failed because there are somethings wrong with the code format. Could you run |
|
@DorianZheng OK, I will run make format and commit it again. |
|
src/rocksdb.rs
Outdated
| } | ||
|
|
||
| pub fn open_cf_with_ttl<'a, T>(opts: DBOptions, path: &str, cfds: Vec<T>, ttls: &[i32]) ->Result<DB, String> |
There was a problem hiding this comment.
OK, I have committed it again.
|
PTAL @DorianZheng @solotzg |
| const char** column_family_names, | ||
| const crocksdb_options_t** column_family_options, | ||
| crocksdb_column_family_handle_t** column_family_handles, | ||
| const int32_t* ttl_array, |
There was a problem hiding this comment.
Please put ttl_array and read_only before column_family_handles, cause it's a input parameter.
FYI: https://google.github.io/styleguide/cppguide.html#Output_Parameters
There was a problem hiding this comment.
This parameter order is according to the declaration of Open function in rocksdb/include/rocksdb/utilities/db_ttl.h. If it is better to conform to google cpp style, I will change it.
src/rocksdb.rs
Outdated
| @@ -441,34 +463,56 @@ impl DB { | |||
| } else { | |||
| false | |||
| }; | |||
| let with_ttl = if ttls.len() > 0 && ttls.len() == cf_names.len() { | |||
There was a problem hiding this comment.
Do you think it's a misuse when ttls.len() != cf_names.len() but ttls.len() > 0). Maybe it's better to throw error and let user to handle it.
|
@fredchenbj The implementation looks good overall. I left some comment. PTAL |
|
@DorianZheng According to your comments, I have fixed both and committed it again. PTAL |
src/rocksdb.rs
Outdated
| if ttls.len() > 0 { | ||
| DB::open_cf_with_ttl(opts, path, cfds, ttls) | ||
| } else { | ||
| DB::open_cf(opts, path, cfds) |
There was a problem hiding this comment.
If someone calls this function without giving ttls, it must be a misuse
There was a problem hiding this comment.
This condition is redundent here, I will remove it. Thus put all ttls conditions in open_cf_internal, and would throw error if not pass .
src/rocksdb.rs
Outdated
| let contains = list.iter().any(|ref cf| cf.is_default()); | ||
| if !contains { | ||
| list.push(ColumnFamilyDescriptor::default()); | ||
| if ttls.len() < list.len() { |
There was a problem hiding this comment.
Only the length of the original list equals to the length of the original ttls, we should add ttl for default column family
There was a problem hiding this comment.
It doesn't matter here, if the length of the original list doesn't equal to the length of the original ttls, it will throw error in open_cf_internal.
There was a problem hiding this comment.
what if user just don't use ttl open here? looks like you will add ttl for default column family here anyway
There was a problem hiding this comment.
yes, this will add ttl, but it doesn't affect for with_ttl would be false, and not use ttl. This logic is ok here, but would not be clear, so I will add condition test here.
There was a problem hiding this comment.
So that we can simplify with_ttl too, it's a little bit confusing. Do you think it's better like:
let with_ttl = if ttls_vec.len() > 0 {
if ttls_vec.len() == cf_names.len() {
true
} else {
return Err("the length of ttls not equal to length of cfs".to_owned());
}
} else {
false
}
There was a problem hiding this comment.
yes, It's better. I will change to like this
let with_ttl = if ttls.len() > 0 { if ttls.len() == cfds.len() && ttls_vec.len() == cf_names.len() { true } else { return Err("the length of ttls not equal to length of cfs".to_owned()); } } else { false }
to make sure be not confusing.
There was a problem hiding this comment.
I have commited it again.
There was a problem hiding this comment.
How about change the condition here FROM:
if ttls.len() < list.len() {
TO
if ttls.len() > 0 {
So that we can get rid of judging if ttls.len() > 0 when deal with with_ttl
There was a problem hiding this comment.
Yes, this way is more clear. I have changed and committed it.
| let p = db.put_cf(cf1, b"k1", b"a"); | ||
| assert!(p.is_ok()); | ||
| } | ||
| } |
There was a problem hiding this comment.
add test for when the length of ttlsis not equal to the length of column_families.
There was a problem hiding this comment.
LGTM, Thank you @fredchenbj for the PR, It's great. @solotzg PTAL
|
Merged. |
Support DBWithTTL::Open interface, and add open_cf_with_ttl in rocksdb.rs