Skip to content

Add rocksdb.iterator.internal-key property #3525

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Add rocksdb.iterator.internal-key property #3525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18421f3
Add "rocksdb.iterator.internal-key" property
sagar0 Feb 20, 2018
011a711
Add a new unit test for a use-case we are trying to address
sagar0 Feb 20, 2018
f00e345
Clarify the comment.
sagar0 Feb 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions db/db_iter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ class DBIter final: public Iterator {
*prop = "Iterator is not valid.";
}
return Status::OK();
} else if (prop_name == "rocksdb.iterator.internal-key") {
*prop = saved_key_.GetUserKey().ToString();
Copy link
Contributor

@siying siying Feb 21, 2018

Choose a reason for hiding this comment

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

Should be ToString(true).

Copy link
Contributor

@siying siying Feb 21, 2018

Choose a reason for hiding this comment

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

Hmm, I guess false may also work.

Copy link
Contributor

@siying siying Feb 21, 2018

Choose a reason for hiding this comment

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

Yes, I think HEX encoding is not necessary, so we should be fine with default, which is false.

Copy link
Contributor Author

@sagar0 sagar0 Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return Status::OK();
}
return Status::InvalidArgument("Undentified property.");
}
Expand Down
49 changes: 49 additions & 0 deletions db/db_iterator_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ TEST_P(DBIteratorTest, IteratorProperty) {
Options options = CurrentOptions();
CreateAndReopenWithCF({"pikachu"}, options);
Put(1, "1", "2");
Delete(1, "2");
ReadOptions ropt;
ropt.pin_data = false;
{
Expand All @@ -90,9 +91,15 @@ TEST_P(DBIteratorTest, IteratorProperty) {
ASSERT_NOK(iter->GetProperty("non_existing.value", &prop_value));
ASSERT_OK(iter->GetProperty("rocksdb.iterator.is-key-pinned", &prop_value));
ASSERT_EQ("0", prop_value);
ASSERT_OK(iter->GetProperty("rocksdb.iterator.internal-key", &prop_value));
ASSERT_EQ("1", prop_value);
iter->Next();
ASSERT_OK(iter->GetProperty("rocksdb.iterator.is-key-pinned", &prop_value));
ASSERT_EQ("Iterator is not valid.", prop_value);

// 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);
Copy link
Contributor

@ajkr ajkr Feb 20, 2018

Choose a reason for hiding this comment

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

isn't this a user key, not an internal key? Internal key should have type and seqnum.

Copy link
Contributor Author

@sagar0 sagar0 Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ajkr ajkr Feb 21, 2018

Choose a reason for hiding this comment

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

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.

}
Close();
}
Expand Down Expand Up @@ -2183,6 +2190,48 @@ TEST_P(DBIteratorTest, SkipStatistics) {
ASSERT_EQ(skip_count, TestGetTickerCount(options, NUMBER_ITER_SKIP));
}

TEST_P(DBIteratorTest, SeekAfterHittingManyInternalKeys) {
Options options = CurrentOptions();
DestroyAndReopen(options);
ReadOptions ropts;
ropts.max_skippable_internal_keys = 2;

Put("1", "val_1");
// Add more tombstones than max_skippable_internal_keys so that Next() fails.
Delete("2");
Delete("3");
Delete("4");
Delete("5");
Put("6", "val_6");

unique_ptr<Iterator> iter(NewIterator(ropts));
iter->SeekToFirst();

ASSERT_TRUE(iter->Valid());
ASSERT_EQ(iter->key().ToString(), "1");
ASSERT_EQ(iter->value().ToString(), "val_1");

// This should fail as incomplete due to too many non-visible internal keys on
// the way to the next valid user key.
iter->Next();
ASSERT_TRUE(!iter->Valid());
ASSERT_TRUE(iter->status().IsIncomplete());

// Get the internal key at which Next() failed.
std::string prop_value;
ASSERT_OK(iter->GetProperty("rocksdb.iterator.internal-key", &prop_value));
ASSERT_EQ("4", prop_value);

// Create a new iterator to seek to the internal key.
unique_ptr<Iterator> iter2(NewIterator(ropts));
iter2->Seek(prop_value);
ASSERT_TRUE(iter2->Valid());
ASSERT_OK(iter2->status());

ASSERT_EQ(iter2->key().ToString(), "6");
ASSERT_EQ(iter2->value().ToString(), "val_6");
}

INSTANTIATE_TEST_CASE_P(DBIteratorTestInstance, DBIteratorTest,
testing::Values(true, false));

Expand Down
3 changes: 3 additions & 0 deletions include/rocksdb/iterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ class Iterator : public Cleanable {
// Property "rocksdb.iterator.super-version-number":
// 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 user-key portion of the internal key at which the iteration
// stopped.
virtual Status GetProperty(std::string prop_name, std::string* prop);

private:
Expand Down