Add GetAllKeyVersions API #2232
Conversation
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr updated the pull request - view changes - changes since last import |
|
LGTM, let's also add a simple test |
utilities/debug.cc
Outdated
| break; | ||
| } | ||
|
|
||
| key_versions->emplace_back(); |
There was a problem hiding this comment.
this is okay but looks a bit strange to me, maybe we can add the constructor and use it in the emplace_back
but it's up to you
|
@ajkr updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
ping, i want to include this in 5.5 so our customer can start using it |
| SequenceNumber sequence; | ||
| // TODO(ajkr): we should provide a helper function that converts the int to a | ||
| // string describing the type for easier debugging. | ||
| int type; |
There was a problem hiding this comment.
the authoritative enum is in db/dbformat.h and I didn't want to duplicate it. What do you suggest - duplicate it anyways since it'll almost never change?
utilities/debug.cc
Outdated
| assert(key_versions != nullptr); | ||
| key_versions->clear(); | ||
|
|
||
| DBImpl* idb = dynamic_cast<DBImpl*>(db->GetRootDB()); |
There was a problem hiding this comment.
Either check whether idb is empty after dynamic casting or just do static cast. I'm voting for simply do static casting.
include/rocksdb/utilities/debug.h
Outdated
| }; | ||
|
|
||
| // Returns listing of all versions of keys in the provided user key range. | ||
| // The range is inclusive-exclusive, i.e., [`begin_key`, `end_key`). |
There was a problem hiding this comment.
I think we should do inclusive end_key. Otherwise if they pass in the same key for begin and end key (and intuitive way if they assume this can return them versions for one specific key), the semantic is confusing.
There was a problem hiding this comment.
the reason for inclusive-exclusive was for compatibility with "ldb idump", which is exclusive of the end key. I can make the function return results for the inclusive-inclusive interval, and then filter out the end key in "ldb idump" to maintain compatibility.
|
@ajkr updated the pull request - view changes - changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
GetAllKeyVersions(), to return a listing of internal data for a range of user keys. The newstruct KeyVersionexposes data similar to internal key without exposing any internal type.