os/bluestore: kvdb histogram #12620

varadakari · 2016-12-22T10:39:06Z

Generates a histogram of key value distribution for kvdb(rocksdb for now)

ifed01 · 2016-12-22T13:43:02Z

src/kv/RocksDBStore.h

@@ -313,6 +313,8 @@ class RocksDBStore : public KeyValueDB {
    bufferlist value();
    bufferptr value_as_ptr();
    int status();
+    size_t key_size();


add 'override' keyword for this and the next declarations?

ifed01 · 2016-12-22T13:44:15Z

src/os/bluestore/BlueStore.h

@@ -1770,6 +1770,7 @@ class BlueStore : public ObjectStore,
  }

  void get_db_statistics(Formatter *f);
+  void get_db_histogram(Formatter *f);


'override' here?

ifed01 · 2016-12-22T13:47:03Z

src/kv/RocksDBStore.h

@@ -150,6 +150,8 @@ class RocksDBStore : public KeyValueDB {

  void close();

+  void split(const std::string &s, char delim, std::vector<std::string> &elems);
+  std::vector<std::string> split(const std::string &s, char delim);


do we really need this form of split?

ifed01 · 2016-12-22T13:47:45Z

src/kv/RocksDBStore.h

@@ -150,6 +150,8 @@ class RocksDBStore : public KeyValueDB {

  void close();

+  void split(const std::string &s, char delim, std::vector<std::string> &elems);


Suggest to rename to split_stats or something to improve readability.

varadakari · 2016-12-23T05:58:17Z

@ifed01 fixed the review comments, please review again.

varadakari · 2016-12-23T05:59:05Z

sample output of the histogram is below:

{
"prefix": "B",
"key-hist": {
"[0,32)": 208,
"max_len": 17,
"value-hist": {
"[0,64)": 208,
"max_len": 16
}
},
"prefix": "C",
"key-hist": {
"[0,32)": 9,
"max_len": 10,
"value-hist": {
"[0,64)": 9,
"max_len": 10
}
},
"prefix": "M",
"key-hist": {
"[0,32)": 47,
"max_len": 26,
"value-hist": {
"[0,64)": 31,
"max_len": 25,
"[128,192)": 8,
"max_len": 186,
"[832,896)": 8,
"max_len": 847
},
"[32,64)": 12807,
"max_len": 42,
"value-hist": {
"[128,192)": 12807,
"max_len": 182
}
},

ifed01

@varadakari sorry for splitting my review into 2 parts - didn't have enough time for complete review yesterday.

ifed01 · 2016-12-23T12:28:52Z

src/os/bluestore/BlueStore.cc

+  //histogram
+  struct value_dist
+  {
+    uint64_t count;


suggest to initialize struct members with 0 here and below.

@ifed01 these are part of the histogram, don't need to specifically initialize to 0, if the value doesn't exist in the map, it will be initialized to 0 by default.

ifed01 · 2016-12-23T12:30:20Z

src/os/bluestore/BlueStore.cc

+    uint64_t count;
+    uint32_t max_len;
+  };
+  typedef struct value_dist value_dist_t;


IMO one can simply define struct value_dist_t above - no need for duplicate type definitions(value_dist & value_dist_t) here.

ifed01 · 2016-12-23T12:30:30Z

src/os/bluestore/BlueStore.cc

+    uint32_t max_len;
+    map<int, value_dist_t> val_map; // slab id to count and others
+  };
+  typedef struct key_dist key_dist_t;


The same here

ifed01 · 2016-12-23T12:32:37Z

src/os/bluestore/BlueStore.cc

+    pair<string,string> key(iter->raw_key());
+
+    if (key.first == PREFIX_SUPER) {
+	key_hist[PREFIX_SUPER][key_slab].count++;


Suggest to introduce a helper func to update a specific key_hist entry in a single call.
e.g.
update_entry( key_hist, PREFIX_SUPER, key_size, value_size);

ifed01 · 2016-12-23T12:35:20Z

src/os/bluestore/BlueStore.cc

+    }
+    else if (key.first == PREFIX_OBJ) {
+      if (key.second.back() == ONODE_KEY_SUFFIX) {
+	key_hist["o"][key_slab].count++;


Suggest to use symbolic names instead of explicit literals: "o", "x", etc

varadakari · 2016-12-26T06:15:15Z

@ifed01 addressed the comments, please review again. Have to move the local variables(maps for the histogram) to make the updates as a function. Otherwise rest of the logic is same.

ifed01

LGTM except code style violation for 'else' statements in void BlueStore::get_db_histogram

varadakari · 2016-12-26T14:38:08Z

@ifed01 Thanks for review, fixed the styling of if-else.

varadakari · 2016-12-27T03:30:28Z

i am not hitting this compilation error in my local copy. will rebase and try it again.

varadakari · 2016-12-28T03:04:13Z

Fixed all the issues and all tests are passing now.

varadakari · 2017-01-09T15:36:08Z

jenkins retest this please

varadakari · 2017-01-11T05:20:29Z

@liewegas could you please take look at this?

liewegas · 2017-01-11T13:30:23Z

src/os/bluestore/BlueStore.cc


+const string PREFIX_ONODE = "o";
+const string PREFIX_ONODE_SHARD = "x";
+const string PREFIX_OTHER = "Z";


Perhaps instead of #defineing more of these we should just use strings ("onode", "other", etc.)?

liewegas · 2017-01-11T13:32:14Z

src/os/bluestore/BlueStore.cc


+map<string, map<int, struct key_dist> > key_hist;
+map<int, uint64_t> value_hist; // only value distribution histogram


Can we put the histogram bits into a class? Should be able to include update_hist_entry?

liewegas · 2017-01-11T13:33:09Z

src/osd/OSD.cc

@@ -1939,6 +1939,8 @@ bool OSD::asok_command(string command, cmdmap_t& cmdmap, string format,
    store->get_db_statistics(f);
  } else if (command == "dump_scrubs") {
    service.dumps_scrub(f);
+  } else if (command == "dump_objectstore_db_histogram") {


It might be a good idea to rename this something like "calculate_objectstore_db_histogram" to better suggest/convey that this is a very slow operation?

Signed-off-by: Varada Kari <varada.kari@sandisk.com>

Improves the presentation of rocksdb dump statistics Signed-off-by: Varada Kari <varada.kari@sandisk.com>

Avoids converting the key and value to string or bufferlist Signed-off-by: Varada Kari <varada.kari@sandisk.com>

varadakari · 2017-01-12T08:37:09Z

@liewegas Made the changes and repushed after rebasing, could you please review again?

varadakari · 2017-01-12T09:23:05Z

jenkins retest this please

liewegas

Few more nits, but otherwise looks good!

liewegas · 2017-01-12T14:20:10Z

src/os/bluestore/BlueStore.cc

+  f->dump_unsigned("total_value_size", total_value_size);
+  f->close_section();
+
+  f->open_object_section("rocksdb value distribution");


no spaces in formatter fields names please

liewegas · 2017-01-12T14:20:28Z

src/os/bluestore/BlueStore.cc

+  f->open_object_section("rocksdb key-value histogram");
+  for (auto i : hist.key_hist) {
+    f->dump_string("prefix", i.first);
+    f->open_object_section("key-hist");


liewegas · 2017-01-12T14:20:58Z

src/os/bluestore/BlueStore.cc

+  f->close_section();
+
+  f->open_object_section("rocksdb key-value histogram");
+  for (auto i : hist.key_hist) {


the dump of the histogram itself should be DBHistogram::dump()

liewegas · 2017-01-12T14:21:16Z

src/os/bluestore/BlueStore.h

@@ -1496,6 +1496,30 @@ class BlueStore : public ObjectStore,
    }
  };

+  struct DBHistogram {
+    struct value_dist
+    {


{ on previous line

liewegas · 2017-01-12T14:21:39Z

src/os/bluestore/BlueStore.h

+    {
+      uint64_t count;
+      uint32_t max_len;
+      map<int, struct value_dist> val_map; // slab id to count and others


these comments should be like ///< so the doc generation can do its thing

Adding a asok to command to dump the key value distribution in a histogram fashion. Signed-off-by: Varada Kari <varada.kari@sandisk.com>

varadakari · 2017-01-13T09:24:03Z

@liewegas Addressed the comments. Please have relook at the changes.

liewegas · 2017-01-13T15:13:53Z

Can you add a test to store_test.cc that triggers this so that the code path is tested?

liewegas · 2017-01-17T14:23:20Z

@varadakari ping

Signed-off-by: Varada Kari <varada.kari@sandisk.com>

varadakari · 2017-01-18T09:14:40Z

@liewegas Added the unit test cases for stats and histogram. Please have a look.

ceph-jenkins · 2017-01-18T09:16:21Z

submodules for project are unmodified

varadakari · 2017-01-24T03:36:43Z

@liewegas if the tests are complete, could you merge this please?

ifed01 reviewed Dec 22, 2016

View reviewed changes

varadakari force-pushed the wip-rocksdb-histogram branch from 44bb025 to 4e56963 Compare December 23, 2016 05:57

ifed01 reviewed Dec 23, 2016

View reviewed changes

varadakari force-pushed the wip-rocksdb-histogram branch from 4e56963 to e8c6d5c Compare December 26, 2016 06:12

varadakari force-pushed the wip-rocksdb-histogram branch from e8c6d5c to 4344490 Compare December 26, 2016 06:19

ifed01 approved these changes Dec 26, 2016

View reviewed changes

varadakari force-pushed the wip-rocksdb-histogram branch from 4344490 to ad07526 Compare December 26, 2016 14:33

varadakari force-pushed the wip-rocksdb-histogram branch from ad07526 to ae36aac Compare December 27, 2016 12:38

liewegas changed the title ~~BlueStore kvdb histogram~~ os/bluestore: kvdb histogram Dec 29, 2016

liewegas added bluestore feature labels Dec 29, 2016

markhpc added the performance label Jan 4, 2017

varadakari force-pushed the wip-rocksdb-histogram branch from ae36aac to 07795bd Compare January 9, 2017 09:22

liewegas requested changes Jan 11, 2017

View reviewed changes

Varada Kari added 3 commits January 12, 2017 13:52

os/bluefs: add interface to dump perf counters to formatter

36bf4f9

Signed-off-by: Varada Kari <varada.kari@sandisk.com>

os/kv: beautify kvdb statistics

8e1d4ac

Improves the presentation of rocksdb dump statistics Signed-off-by: Varada Kari <varada.kari@sandisk.com>

os/kv: add interface to size of key and value from wholespaceiter

1b1018d

Avoids converting the key and value to string or bufferlist Signed-off-by: Varada Kari <varada.kari@sandisk.com>

varadakari force-pushed the wip-rocksdb-histogram branch from 07795bd to c877843 Compare January 12, 2017 08:31

liewegas requested changes Jan 12, 2017

View reviewed changes

os/bluestore: interface to dump key value distribution in kvdb

64af217

Adding a asok to command to dump the key value distribution in a histogram fashion. Signed-off-by: Varada Kari <varada.kari@sandisk.com>

varadakari force-pushed the wip-rocksdb-histogram branch from c877843 to 64af217 Compare January 13, 2017 05:59

liewegas approved these changes Jan 13, 2017

View reviewed changes

liewegas added the needs-qa label Jan 13, 2017

liewegas removed the needs-qa label Jan 13, 2017

Varada Kari added 2 commits January 18, 2017 14:28

os/bluestore: Add unittest to test kvdb histogram

5a0d639

Signed-off-by: Varada Kari <varada.kari@sandisk.com>

os/bluestore: Add unittest to test kvdb dump stats

eb35611

Signed-off-by: Varada Kari <varada.kari@sandisk.com>

liewegas added the needs-qa label Jan 18, 2017

liewegas added the wip-sage-testing label Jan 24, 2017

liewegas merged commit 801fd3c into ceph:master Jan 25, 2017

varadakari deleted the wip-rocksdb-histogram branch January 26, 2017 03:39

tchaikov mentioned this pull request Aug 12, 2015

doc/rados/operations/add-or-rm-mons: simplify the steps to add a mon #5495

Merged

		@@ -150,6 +150,8 @@ class RocksDBStore : public KeyValueDB {

		void close();

		void split(const std::string &s, char delim, std::vector<std::string> &elems);


		map<string, map<int, struct key_dist> > key_hist;
		map<int, uint64_t> value_hist; // only value distribution histogram

os/bluestore: kvdb histogram #12620

os/bluestore: kvdb histogram #12620

Conversation

varadakari commented Dec 22, 2016

ifed01 Dec 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varadakari commented Dec 23, 2016

varadakari commented Dec 23, 2016

ifed01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varadakari commented Dec 26, 2016

ifed01 left a comment

Choose a reason for hiding this comment

varadakari commented Dec 26, 2016

varadakari commented Dec 27, 2016

varadakari commented Dec 28, 2016

varadakari commented Jan 9, 2017

varadakari commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varadakari commented Jan 12, 2017

varadakari commented Jan 12, 2017

liewegas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varadakari commented Jan 13, 2017

liewegas commented Jan 13, 2017

liewegas commented Jan 17, 2017

varadakari commented Jan 18, 2017

ceph-jenkins commented Jan 18, 2017

varadakari commented Jan 24, 2017

ifed01 Dec 22, 2016 •

edited