Add kOptionsStatistics to GetProperty() #3966

zhichao-cao · 2018-06-07T05:58:23Z

Add a new DB property to DB::GetProperty(), which returns the option.statistics. Test is updated to pass.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2018-06-07T16:58:30Z

include/rocksdb/db.h

+
+    // "rocksdb.immutable_db_options_statistic" - returns the statistic
+    //      of immutable db options
+    static const std::string kImmutableDBOptionStatistic;


This name is not clear enough for people who operates this system. They may not know what immutable_db_options is. immutable_db_options is in fact an internal data structure. Telling people the location of the stat object is not helpful.

We usually call this object statistics. Maybe we can just call it rocksdb.statistics. I know it's a little bit confusing becasue we already have "rocksdb.stats" and "rocksdb.dbstats". If you think it's too confusing, something like rocksdb.statistics.in.options will be better than the name it is currently picked.

facebook-github-bot · 2018-06-07T23:37:49Z

@zhichao-cao has updated the pull request.

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2018-06-08T00:44:42Z

db/db_impl.cc

@@ -1845,6 +1845,14 @@ bool DBImpl::GetProperty(ColumnFamilyHandle* column_family,
    InstrumentedMutexLock l(&mutex_);
    return cfd->internal_stats()->GetStringProperty(*property_info, property,
                                                    value);
+  } else if (property_info->is_immutable_db_statistic) {
+    assert(value != nullptr);
+    auto dbstats = immutable_db_options_.statistics.get();


Google C++ Style discourage usage of auto like this: https://google.github.io/styleguide/cppguide.html#auto

siying · 2018-06-08T00:53:01Z

db/internal_stats.h

@@ -45,6 +45,9 @@ struct DBPropertyInfo {

  // @param props Map of general properties to populate
  bool (InternalStats::*handle_map)(std::map<std::string, std::string>* props);
+
+  // special case to get the immutable db statistics
+  bool is_immutable_db_statistic;


It doesn't create much value to parse to this bool and still do the work in DBImpl::GetProperty() info.
Either we still provide a callback here with type like

bool (DBImpl::handle_string_dbimpl)(std::string value);

Or you do the parsing and execution all in DBImpl::GetProperty(), before calling GetPropertyInfo().

facebook-github-bot · 2018-06-08T17:31:38Z

@zhichao-cao has updated the pull request.

siying

Can you change the title of the PR to include the property name? At the same time, I think we don't have to mention immutable_db_options_.statistics.

facebook-github-bot · 2018-06-08T17:47:10Z

@zhichao-cao has updated the pull request.

siying · 2018-06-08T17:45:56Z

db/db_impl.cc

@@ -1911,6 +1918,16 @@ bool DBImpl::GetIntPropertyInternal(ColumnFamilyData* cfd,
  }
 }

+bool DBImpl::HandleImmutableOptionStatistics(std::string* value) {
+  assert(value != nullptr);
+  auto dbstats = immutable_db_options_.statistics.get();


auto is not suggested in the usage like this. See https://google.github.io/styleguide/cppguide.html#auto

siying · 2018-06-08T17:49:07Z

db/db_impl.h

@@ -1379,6 +1379,7 @@ class DBImpl : public DB {
  bool GetIntPropertyInternal(ColumnFamilyData* cfd,
                              const DBPropertyInfo& property_info,
                              bool is_locked, uint64_t* value);
+  bool HandleImmutableOptionStatistics(std::string* value);


This is a function of DBImpl. This function name is not clear enough under this class. Mention something related to property. Also, I suggest we don't mention "ImmutableOption" here. statistics is statistics. The reason it is in ImmutableOption, is that we decided to currently put it there. It can be moved as we wish in the future. If we do that, we have to remember to update this function name. Otherwise, it will be invalid.

facebook-github-bot · 2018-06-08T18:08:16Z

@zhichao-cao has updated the pull request. View: changes, changes since last import

siying · 2018-06-08T18:12:00Z

db/db_impl.h

@@ -1379,6 +1379,7 @@ class DBImpl : public DB {
  bool GetIntPropertyInternal(ColumnFamilyData* cfd,
                              const DBPropertyInfo& property_info,
                              bool is_locked, uint64_t* value);
+  bool HandleDBStatisticsInOptions(std::string* value);


It still doesn't tell the function is to handle get property requests.

facebook-github-bot · 2018-06-08T18:28:24Z

@zhichao-cao has updated the pull request.

ajkr · 2018-06-08T19:44:27Z

db/internal_stats.cc

          nullptr}},
+        {DB::Properties::kDBStatisticsInOptions,


this naming is confusing. They aren't really DB statistics since the same Statistics object can be shared across DBs. How about calling it kStatistics?

Thanks for the suggestion and now it is changed to kStatistics.

All names are updated to **OptionsStatistics. Comments are changed too.

…b_options_.statistics->ToString(), if it is not empty.

Summary: Add a new DB property to DB::GetProperty(), which returns immutable_db_options_.statistics->ToString(), if it is not empty. Test Plan: Added the unite test for this property, run asan_check and check Reviewers: Siying Dong Subscribers: Tasks:T30165586 Tags:

Summary: Add a new DB property to DB::GetProperty(), which returns immutable_db_options_.statistics->ToString(), if it is not empty. Using the DBimpl callback and setup a new category Test Plan: Added the unite test for this property, run asan_check and check Reviewers: Siying Dong Subscribers: Tasks:T30165586 Tags:

Summary: Add a new DB property to DB::GetProperty(), which returns the statistics of the DB in Options, if it is not empty. Using the DBimpl callback and setup a new category. Fixed the naming and format issues Test Plan:Added the unite test for this property, run asan_check and check Reviewers: Subscribers: Tasks: Tags:

Summary: Add a new DB property to DB::GetProperty(), which returns the statistics of the DB in Options, if it is not empty. Using the DBimpl callback and setup a new category. Fixed the naming and format issues Test Plan: Added the unite test for this property, run asan_check and check Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2018-06-08T21:38:54Z

@zhichao-cao has updated the pull request.

siying · 2018-06-08T21:53:05Z

include/rocksdb/db.h

@@ -621,6 +621,10 @@ class DB {
    // "rocksdb.block-cache-pinned-usage" - returns the memory size for the
    //      entries being pinned.
    static const std::string kBlockCachePinnedUsage;
+
+    // "rocksdb.db_statistics_in_options" - returns multi-line string


Fix this comment.

siying · 2018-06-08T21:53:42Z

include/rocksdb/db.h

@@ -621,6 +621,10 @@ class DB {
    // "rocksdb.block-cache-pinned-usage" - returns the memory size for the
    //      entries being pinned.
    static const std::string kBlockCachePinnedUsage;
+
+    // "rocksdb.db_statistics_in_options" - returns multi-line string
+    //      containing the statistics of the db in options


Maybe saying "options.statistics", rather than "the statistics of the db in the options".

siying · 2018-06-08T21:54:04Z

db/internal_stats.cc

@@ -249,6 +249,7 @@ static const std::string estimate_oldest_key_time = "estimate-oldest-key-time";
 static const std::string block_cache_capacity = "block-cache-capacity";
 static const std::string block_cache_usage = "block-cache-usage";
 static const std::string block_cache_pinned_usage = "block-cache-pinned-usage";
+static const std::string db_statistics_in_options = "db-statistics-in-options";


facebook-github-bot · 2018-06-08T22:32:02Z

@zhichao-cao has updated the pull request. View: changes, changes since last import

siying · 2018-06-08T22:33:29Z

include/rocksdb/db.h

@@ -621,6 +621,10 @@ class DB {
    // "rocksdb.block-cache-pinned-usage" - returns the memory size for the
    //      entries being pinned.
    static const std::string kBlockCachePinnedUsage;
+
+    // "rocksdb.options-statistics" - returns multi-line string


This confuses me that the property name kStatistics = "rocksdb.options-statistics".

facebook-github-bot · 2018-06-08T23:15:06Z

@zhichao-cao has updated the pull request. View: changes, changes since last import

siying

Please make sure @ajkr is OK with this new name before landing it.

siying · 2018-06-08T23:21:50Z

db/internal_stats.h

@@ -45,6 +45,10 @@ struct DBPropertyInfo {

  // @param props Map of general properties to populate
  bool (InternalStats::*handle_map)(std::map<std::string, std::string>* props);
+
+  // handle the private or protected property info of DB


What does "private or protected" mean?

If the property is not the private or protected member of DBImpl, they can be directly handled by GetIntProperty or GetStringProperty. Only the private or protected members's property need to use the DBImpl callback to return the value.

siying · 2018-06-08T23:23:56Z

include/rocksdb/db.h

@@ -621,6 +621,10 @@ class DB {
    // "rocksdb.block-cache-pinned-usage" - returns the memory size for the
    //      entries being pinned.
    static const std::string kBlockCachePinnedUsage;
+
+    // "rocksdb.options-statistics" - returns multi-line string
+    //      containing options.statistics


"containing options.statistics" feels a little bit odd to me. Maybe "for options.statistics", or "of options.statistics"?

I will fix it, using "of options.statistics"

facebook-github-bot · 2018-06-09T00:04:55Z

@zhichao-cao has updated the pull request. View: changes, changes since last import

facebook-github-bot · 2018-06-09T00:07:45Z

@zhichao-cao has updated the pull request. View: changes, changes since last import

siying

Looks good to me. Good job!

facebook-github-bot

@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2018-06-15T20:37:10Z

@zhichao-cao are you going to land it?

zhichao-cao · 2018-06-15T23:16:35Z

@siying Sorry, I forgot this one has been approved. I will land it now.

facebook-github-bot

@zhichao-cao is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Add a new DB property to DB::GetProperty(), which returns the option.statistics. Test is updated to pass. Closes facebook#3966 Differential Revision: D8311139 Pulled By: zhichao-cao fbshipit-source-id: ea78f4727358c807b0e5a0ea62e09defb10ad9ac

facebook-github-bot added the CLA Signed label Jun 7, 2018

facebook-github-bot reviewed Jun 7, 2018

View reviewed changes

siying suggested changes Jun 7, 2018

View reviewed changes

zhichao-cao force-pushed the add_CompressionOptions branch from 5fed6bb to e4738b7 Compare June 7, 2018 23:37

facebook-github-bot reviewed Jun 7, 2018

View reviewed changes

siying reviewed Jun 8, 2018

View reviewed changes

zhichao-cao force-pushed the add_CompressionOptions branch from e4738b7 to 90ff99d Compare June 8, 2018 17:24

zhichao-cao force-pushed the add_CompressionOptions branch from 90ff99d to 520a85e Compare June 8, 2018 17:38

siying reviewed Jun 8, 2018

View reviewed changes

zhichao-cao changed the title ~~Add immutable_db_options_.statistics to GetProperty()~~ Add kDBStatisticsInOptions to GetProperty() Jun 8, 2018

siying reviewed Jun 8, 2018

View reviewed changes

zhichao-cao force-pushed the add_CompressionOptions branch from f870a67 to d99c5d5 Compare June 8, 2018 18:28

ajkr reviewed Jun 8, 2018

View reviewed changes

zhichao-cao added 5 commits June 8, 2018 14:18

Add a new DB property to DB::GetProperty(), which returns immutable_d…

e71d3b9

…b_options_.statistics->ToString(), if it is not empty.

zhichao-cao force-pushed the add_CompressionOptions branch from d99c5d5 to 10835ad Compare June 8, 2018 21:34

zhichao-cao changed the title ~~Add kDBStatisticsInOptions to GetProperty()~~ Add kStatistics to GetProperty() Jun 8, 2018

siying reviewed Jun 8, 2018

View reviewed changes

Changed the property naming and function name in DBimpl

d7c6f58

siying reviewed Jun 8, 2018

View reviewed changes

Changed the property name to kOptionsStatistics

8427698

zhichao-cao changed the title ~~Add kStatistics to GetProperty()~~ Add kOptionsStatistics to GetProperty() Jun 8, 2018

siying approved these changes Jun 8, 2018

View reviewed changes

Changed the comment

0bba9f6

changed the comments

760af46

siying approved these changes Jun 9, 2018

View reviewed changes

facebook-github-bot reviewed Jun 9, 2018

View reviewed changes

facebook-github-bot reviewed Jun 15, 2018

View reviewed changes

facebook-github-bot closed this in 3fbc865 Jun 16, 2018

Add kOptionsStatistics to GetProperty() #3966

Add kOptionsStatistics to GetProperty() #3966

Conversation

zhichao-cao commented Jun 7, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 7, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

siying left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2018

siying 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

facebook-github-bot commented Jun 9, 2018

facebook-github-bot commented Jun 9, 2018

siying left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Jun 15, 2018

zhichao-cao commented Jun 15, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

zhichao-cao commented Jun 7, 2018 •

edited

Loading