Promote rocksdb.{deleted.keys,merge.operands} to main table properties #4594
Conversation
There was a problem hiding this comment.
abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This removes part of the public API, so I'm going to refactor this a bit to both support the new |
|
@abhimadan has updated the pull request. Re-import the pull request |
|
Added back support for the old API. Should be ready for review now. |
There was a problem hiding this comment.
abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@abhimadan has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@abhimadan has updated the pull request. Re-import the pull request |
Summary: Since the number of range deletions are reported in TableProperties, it is confusing to not report the number of merge operands and point deletions as top-level properties; they are accessible through the public API, but since they are not the "main" properties, they do not appear in aggregated table properties, or the string representation of table properties. This change promotes those two property keys to `rocksdb/table_properties.h`, adds corresponding uint64 members for them, deprecates the old access methods `GetDeletedKeys()` and `GetMergeOperands()`, and entirely removes `InternalKeyTablePropertiesCollector`. Test Plan: make check
abhimadan
force-pushed
the
report-deleted-keys
branch
from
fb83e66
to
6205718
Compare
|
@abhimadan has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -216,6 +222,10 @@ struct TableProperties { | |||
| // Below is a list of non-basic properties that are collected by database | |||
| // itself. Especially some properties regarding to the internal keys (which | |||
| // is unknown to `table`). | |||
| // | |||
| // DEPRECATED: these properties now belong as TableProperties members. Please | |||
There was a problem hiding this comment.
Good, we can remove these functions in the next major release (6.0).
Summary: Since the number of range deletions are reported in
TableProperties, it is confusing to not report the number of merge
operands and point deletions as top-level properties; they are
accessible through the public API, but since they are not the "main"
properties, they do not appear in aggregated table properties, or the
string representation of table properties.
This change promotes those two property keys to
rocksdb/table_properties.h, adds corresponding uint64 members forthem, deprecates the old access methods
GetDeletedKeys()andGetMergeOperands()(though they are still usable for now), and removesInternalKeyPropertiesCollector. The property key strings are the sameas before this change, so this should be able to read DBs written from older
versions (though I haven't tested this yet).
Test Plan: make check