Add rocksdb.iterator.internal-key property #3525
Conversation
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
|
||
| // Get internal key at which the iteration stopped (tombstone in this case). | ||
| ASSERT_OK(iter->GetProperty("rocksdb.iterator.internal-key", &prop_value)); | ||
| ASSERT_EQ("2", prop_value); |
There was a problem hiding this comment.
isn't this a user key, not an internal key? Internal key should have type and seqnum.
There was a problem hiding this comment.
internal non-user-exposed key converted to a user key. I am not exposing the full format ie. with the type and seqnum, but just the actual key part of it so that a user can use it in future operations (like Seek).
I didn't want to name it rocksdb.iterator.key as it could cause confusion with key(). So I just named it internal-key instead. Do you have a better name in mind? I could rename it to something else if you prefer.
There was a problem hiding this comment.
Yeah it is hard to name. Only thing I can think of is something like "subiterator-key" to indicate it comes from the lower-level iterator, which may or may not be visible to users. Will leave it up to you if you want to change it.
ajkr
left a comment
ajkr
left a comment
There was a problem hiding this comment.
lgtm, leave the naming as-is is fine with me.
include/rocksdb/iterator.h
Outdated
| // LSM version used by the iterator. The same format as DB Property | ||
| // kCurrentSuperVersionNumber. See its comment for more information. | ||
| // Property "rocksdb.iterator.internal-key": | ||
| // Get the internal key (e.g. tombstone) at which the iteration stopped. |
There was a problem hiding this comment.
maybe at least clarify it's the user-key portion here, so user wouldn't know if it's tombstone or not (e.g., maybe it's an old version of a key covered by a tombstone)
There was a problem hiding this comment.
Updated the comment.
| } | ||
| return Status::OK(); | ||
| } else if (prop_name == "rocksdb.iterator.internal-key") { | ||
| *prop = saved_key_.GetUserKey().ToString(); |
There was a problem hiding this comment.
Hmm, I guess false may also work.
There was a problem hiding this comment.
Yes, I think HEX encoding is not necessary, so we should be fine with default, which is false.
There was a problem hiding this comment.
I don't think it is necessary to be in hex, as we don't necessarily need it to be printed. string itself can contain un-printable characters (including null). I verified that we are indeed using the right string api based on the size of data, instead of just a regular copy (which stops the first time you encounter a null character):
https://github.com/facebook/rocksdb/blob/master/util/slice.cc#L158 .
If you actually need the key to be printed, then, yes, I agree that it needs to be in hex.
I tested it earlier that we are indeed returning the full slice value converted to a string, for example, a string built something like:
char tmp[] = {10, 0, 15, 50}
String key(tmp, 4);
This gets returned correctly via the property.
|
@sagar0 has updated the pull request. View: changes, changes since last import |
|
I don't have any further comment. Defer to @ajkr to access after his comment is addressed. |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sagar0 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Added a new iterator property: `rocksdb.iterator.internal-key` to get the internal-key (converted to user key) at which the iterator stopped. Closes #3525 Differential Revision: D7033694 Pulled By: sagar0 fbshipit-source-id: d51e6c00f5e9d766c6276ef79774b81c6c5216f8
Summary: Added a new iterator property: `rocksdb.iterator.internal-key` to get the internal-key (converted to user key) at which the iterator stopped. Closes #3525 Differential Revision: D7033694 Pulled By: sagar0 fbshipit-source-id: d51e6c00f5e9d766c6276ef79774b81c6c5216f8
4 participants
Added a new iterator property:
rocksdb.iterator.internal-keyto get the internal-key (converted to user key) at which the iterator stopped.Test Plan:
Added a new unit-test and modified an existing one.
make check -j64