Skip to content
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.

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

kv/KeyValueDB: add column family #18049

Merged
merged 11 commits into from Oct 6, 2017
Merged

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Sep 29, 2017

This is another iteration of Adam's squash, but it also cleans up the iterator
mess in KeyValueDB, and simplifies a bunch of the cf handle stuff in rocksdb.

  • cleaned up iterators (for KeyValueDB and also the CF one)
  • detect existing CFs on open
  • add bluestore option to create a CF for deferred and omap

class TransactionImpl {
public:
/// Set Keys
void set(
const std::string &prefix, ///< [in] Prefix for keys
const std::string &prefix, ///< [in] Prefix for keys, or CF name

Choose a reason for hiding this comment

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

tab left

@liewegas liewegas force-pushed the wip-kv-iterator branch 2 times, most recently from 5ac15ce to 01814ea Compare October 2, 2017 20:51
@liewegas
Copy link
Member Author

liewegas commented Oct 2, 2017

@markhpc can you give this one a whirl? I think it's basically ready to merge.

@markhpc
Copy link
Member

markhpc commented Oct 3, 2017

On it!

liewegas and others added 6 commits October 3, 2017 13:46
These iterfaces are similar but ultimately unrelated! Unfortunately
we are stuck with a joined class hiearchy because of DBObjectMap.
Someday it would be nice to break this link.

Signed-off-by: Sage Weil <sage@redhat.com>
This uses the most general class as the type for the Iterator
shared_ptr typedef, and makes the class that filters by prefix
private.

Signed-off-by: Sage Weil <sage@redhat.com>
get_wholespace_iterator() and get_iterator().  This gets rid of
the goofy _get_iterator() indirection and avoids colliding on names w/
virtual methods.

Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Jianjian Huo <jianjian.huo@ssi.samsung.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Jianjian Huo and others added 4 commits October 4, 2017 22:01
- detect and use existing CFs on open

Signed-off-by: Jianjian Huo <jianjian.huo@ssi.samsung.com>
Signed-off-by: Adam C. Emerson <aemerson@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Merge operators always map to a prefix, just like a CF, so we can link
directly to the operator without doing any lookup.

Signed-off-by: Sage Weil <sage@redhat.com>
This was hard-coded to 'b'.

Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
Copy link
Member

@markhpc markhpc left a comment

Choose a reason for hiding this comment

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

Overall this PR is working well during testing. No obvious performance differences in the tests I've run and the extra compaction information I believe will be quite useful. Only one minor comment below.

@@ -65,8 +65,14 @@ const string PREFIX_OBJ = "O"; // object name -> onode_t
const string PREFIX_OMAP = "M"; // u64 + keyname -> value
const string PREFIX_DEFERRED = "L"; // id -> deferred_transaction_t
const string PREFIX_ALLOC = "B"; // u64 offset -> u64 length (freelist)
const string PREFIX_ALLOC_BITMAP = "b"; // (see BitmapFreelistManager)
Copy link
Member

Choose a reason for hiding this comment

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

The old way of doing this is nasty, but I'm not sure it's great to move BitmapFreelistManager specific prefix into BlueStore.cc?

OTOH, we can just reserve "B" and "b" for freelist management in general, which feels a bit saner, so maybe this is ok in the end.

We need to close the default CF handle *only* if we explicitly opened it.

Also, take care to use that handle wherever we can.  This is mostly
paranoia--I'm not sure if rocksdb cares if you mix the handle you opened
with DefaultColumnFamily(), which is different.

Signed-off-by: Sage Weil <sage@redhat.com>
Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

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

Let's get this in so i can rebase things onto it.

@liewegas liewegas merged commit b73346d into ceph:master Oct 6, 2017
@liewegas liewegas deleted the wip-kv-iterator branch October 6, 2017 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants