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

[kvdb-rocksdb] adds num_keys() #285

Merged
merged 9 commits into from
Dec 19, 2019
Merged

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Dec 17, 2019

Uses "rocksdb.estimate-num-keys" to get an estimate of the number of keys in a database (see https://github.com/facebook/rocksdb/wiki/RocksDB-FAQ#failure-recovery).

Uses `"rocksdb.estimate-num-keys"` to get an estimate of the number of keys in a database (see https://github.com/facebook/rocksdb/wiki/RocksDB-FAQ#failure-recovery).
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Do you think it makes sense to add it to the kvdb trait?

kvdb-rocksdb/src/lib.rs Outdated Show resolved Hide resolved
@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 17, 2019

Do you think it makes sense to add it to the kvdb trait?

That's what I did in the other PR but I think I'd like to see a use case outside rocksdb first. Databases vary a lot when it comes to precision and performance of counting the records so not sure it's something we should abstract over. E.g. the memory db impl would likely not be an estimate, and who knows how lmdb works in this area?

@dvdplm dvdplm requested a review from ordian December 19, 2019 07:30
kvdb-rocksdb/src/lib.rs Outdated Show resolved Hide resolved
kvdb-rocksdb/src/lib.rs Outdated Show resolved Hide resolved
Co-Authored-By: Andronik Ordian <write@reusable.software>
Some(ref cfs) => {
let cf = cfs.cf(col as usize);
match cfs.db.property_int_value_cf(cf, ESTIMATE_NUM_KEYS) {
Ok(estimate) => Ok(estimate.unwrap_or_default()),
Copy link
Member

@niklasad1 niklasad1 Dec 19, 2019

Choose a reason for hiding this comment

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

hmm, I'm not sure that I follow the propagation of errors here.

If no db exist => Ok(0)
Ok(None) => only happens on bad &str?!
Err(e) => parsing of number failed or something else?

according to https://github.com/facebook/rocksdb/blob/08809f5e6cd9cc4bc3958dd4d59457ae78c76660/include/rocksdb/db.h#L654-L689 "rocksdb.estimate-num-keys" will succeed so I see no problem with but some errors/Option::None might get swallowed

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

indentation in the changelog should be fixed except that it looks good,

@dvdplm dvdplm merged commit 73297de into master Dec 19, 2019
@dvdplm dvdplm deleted the dp/feature/add-num-entries-to-rocksdb branch December 19, 2019 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants