Added SizeApproximationOptions to DB::GetApproximateSizes #5626
Conversation
elipoz
force-pushed
the
GetApproximateSizes
branch
from
3967281
to
ce65d9b
Compare
elipoz
changed the title
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
include/rocksdb/db.h
Outdated
| // data in the mem-tables (if the mem-table type supports it), data | ||
| // serialized to disk, or both. | ||
| SizeApproximationFlags include_flags = INCLUDE_FILES; | ||
| }; |
There was a problem hiding this comment.
We put all those options in file options.h. Maybe we should find a good home to all those in the future, but right now I suggest we keep it there.
include/rocksdb/db.h
Outdated
| // Defines whether the returned size should include the recently written | ||
| // data in the mem-tables (if the mem-table type supports it), data | ||
| // serialized to disk, or both. | ||
| SizeApproximationFlags include_flags = INCLUDE_FILES; |
There was a problem hiding this comment.
It's annoying that we started with a bitmap. Now looks like what we should do is to have variable bool include_memtables and bool include_files in SizeApproximationOptions, other than this two levels of data structures. Can we do something like that?
elipoz
force-pushed
the
GetApproximateSizes
branch
from
ce65d9b
to
4a3f386
Compare
|
@elipoz has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
include/rocksdb/db.h
Outdated
| virtual void GetApproximateSizes(ColumnFamilyHandle* column_family, | ||
| const Range* range, int n, uint64_t* sizes, | ||
| uint8_t include_flags = INCLUDE_FILES) = 0; | ||
| const SizeApproximationOptions& options) = 0; |
There was a problem hiding this comment.
Option is usually the first argument of the function. We should keep it there too. Since we are changing the function, it's very odd that we don't return Status while there can be underlying I/O error. How about add a return Status to this function?
elipoz
force-pushed
the
GetApproximateSizes
branch
from
4a3f386
to
a9920b4
Compare
| const Range* range, int n, uint64_t* sizes) { | ||
| if (!options.include_memtabtles && !options.include_files) { | ||
| return Status::InvalidArgument("Invalid options"); | ||
| } |
There was a problem hiding this comment.
Since we change the behavior from asserting to return a status, we should mention it as a public interface change in HISTORY.md
elipoz
force-pushed
the
GetApproximateSizes
branch
from
a9920b4
to
ebdc21e
Compare
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
) Summary: The new DB::GetApproximateSizes with SizeApproximationOptions argument, which allows to add more options/knobs to the DB::GetApproximateSizes call (beyond only the include_flags) Pull Request resolved: facebook#5626 Differential Revision: D16496913 Pulled By: elipoz fbshipit-source-id: ee8c6c182330a285fa056ecfc3905a592b451720
The new DB::GetApproximateSizes with SizeApproximationOptions argument, which allows to add more options/knobs to the DB::GetApproximateSizes call (beyond only the include_flags)