Eliminate duplicated property constants

Summary:
Before this diff, there were duplicated constants to refer to properties (user-
facing API had strings and InternalStats had an enum). I noticed these were
inconsistent in terms of which constants are provided, names of constants, and
documentation of constants. Overall it seemed annoying/error-prone to maintain
these duplicated constants.

So, this diff gets rid of InternalStats's constants and replaces them with a map
keyed on the user-facing constant. The value in that map contains a function
pointer to get the property value, so we don't need to do string matching while
holding db->mutex_. This approach has a side benefit of making many small
handler functions rather than a giant switch-statement.

Test Plan: db_properties_test passes, running "make commit-prereq -j32"

Reviewers: sdong, yhchiang, kradhakrishnan, IslamAbdelRahman, rven, anthony

Reviewed By: anthony

Subscribers: dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D53253

db/db_impl.cc

@@ -494,23 +494,22 @@ void DBImpl::MaybeDumpStats() {
     last_stats_dump_time_microsec_ = now_micros;
 
 #ifndef ROCKSDB_LITE
-    bool tmp1 = false;
-    bool tmp2 = false;
-    DBPropertyType cf_property_type =
-        GetPropertyType(DB::Properties::kCFStats, &tmp1, &tmp2);
-    DBPropertyType db_property_type =
-        GetPropertyType(DB::Properties::kDBStats, &tmp1, &tmp2);
+    const DBPropertyInfo* cf_property_info =
+        GetPropertyInfo(DB::Properties::kCFStats);
+    assert(cf_property_info != nullptr);
+    const DBPropertyInfo* db_property_info =
+        GetPropertyInfo(DB::Properties::kDBStats);
+    assert(db_property_info != nullptr);
+
     std::string stats;
     {
       InstrumentedMutexLock l(&mutex_);
       for (auto cfd : *versions_->GetColumnFamilySet()) {
-        cfd->internal_stats()->GetStringProperty(cf_property_type,
-                                                 DB::Properties::kCFStats,
-                                                 &stats);
+        cfd->internal_stats()->GetStringProperty(
+            *cf_property_info, DB::Properties::kCFStats, &stats);
       }
-      default_cf_internal_stats_->GetStringProperty(db_property_type,
-                                                    DB::Properties::kDBStats,
-                                                    &stats);
+      default_cf_internal_stats_->GetStringProperty(
+          *db_property_info, DB::Properties::kDBStats, &stats);
     }
     Log(InfoLogLevel::WARN_LEVEL,
         db_options_.info_log, "------- DUMPING STATS -------");
@@ -4701,53 +4700,51 @@ const DBOptions& DBImpl::GetDBOptions() const { return db_options_; }
 
 bool DBImpl::GetProperty(ColumnFamilyHandle* column_family,
                          const Slice& property, std::string* value) {
-  bool is_int_property = false;
-  bool need_out_of_mutex = false;
-  DBPropertyType property_type =
-      GetPropertyType(property, &is_int_property, &need_out_of_mutex);
-
+  const DBPropertyInfo* property_info = GetPropertyInfo(property);
   value->clear();
   auto cfd = reinterpret_cast<ColumnFamilyHandleImpl*>(column_family)->cfd();
-  if (is_int_property) {
+  if (property_info == nullptr) {
+    return false;
+  } else if (property_info->handle_int) {
     uint64_t int_value;
-    bool ret_value = GetIntPropertyInternal(
-        cfd, property_type, need_out_of_mutex, false, &int_value);
+    bool ret_value =
+        GetIntPropertyInternal(cfd, *property_info, false, &int_value);
     if (ret_value) {
       *value = ToString(int_value);
     }
     return ret_value;
-  } else {
+  } else if (property_info->handle_string) {
     InstrumentedMutexLock l(&mutex_);
-    return cfd->internal_stats()->GetStringProperty(property_type, property,
+    return cfd->internal_stats()->GetStringProperty(*property_info, property,
                                                     value);
   }
+  // Shouldn't reach here since exactly one of handle_string and handle_int
+  // should be non-nullptr.
+  assert(false);
+  return false;
 }
 
 bool DBImpl::GetIntProperty(ColumnFamilyHandle* column_family,
                             const Slice& property, uint64_t* value) {
-  bool is_int_property = false;
-  bool need_out_of_mutex = false;
-  DBPropertyType property_type =
-      GetPropertyType(property, &is_int_property, &need_out_of_mutex);
-  if (!is_int_property) {
+  const DBPropertyInfo* property_info = GetPropertyInfo(property);
+  if (property_info == nullptr || property_info->handle_int == nullptr) {
     return false;
   }
   auto cfd = reinterpret_cast<ColumnFamilyHandleImpl*>(column_family)->cfd();
-  return GetIntPropertyInternal(cfd, property_type, need_out_of_mutex, false,
-                                value);
+  return GetIntPropertyInternal(cfd, *property_info, false, value);
 }
 
 bool DBImpl::GetIntPropertyInternal(ColumnFamilyData* cfd,
-                                    DBPropertyType property_type,
-                                    bool need_out_of_mutex, bool is_locked,
-                                    uint64_t* value) {
-  if (!need_out_of_mutex) {
+                                    const DBPropertyInfo& property_info,
+                                    bool is_locked, uint64_t* value) {
+  assert(property_info.handle_int != nullptr);
+  if (!property_info.need_out_of_mutex) {
     if (is_locked) {
       mutex_.AssertHeld();
-      return cfd->internal_stats()->GetIntProperty(property_type, value, this);
+      return cfd->internal_stats()->GetIntProperty(property_info, value, this);
     } else {
       InstrumentedMutexLock l(&mutex_);
-      return cfd->internal_stats()->GetIntProperty(property_type, value, this);
+      return cfd->internal_stats()->GetIntProperty(property_info, value, this);
     }
   } else {
     SuperVersion* sv = nullptr;
@@ -4758,7 +4755,7 @@ bool DBImpl::GetIntPropertyInternal(ColumnFamilyData* cfd,
     }
 
     bool ret = cfd->internal_stats()->GetIntPropertyOutOfMutex(
-        property_type, sv->current, value);
+        property_info, sv->current, value);
 
     if (!is_locked) {
       ReturnAndCleanupSuperVersion(cfd, sv);
@@ -4770,11 +4767,8 @@ bool DBImpl::GetIntPropertyInternal(ColumnFamilyData* cfd,
 
 bool DBImpl::GetAggregatedIntProperty(const Slice& property,
                                       uint64_t* aggregated_value) {
-  bool need_out_of_mutex;
-  bool is_int_property;
-  DBPropertyType property_type =
-      GetPropertyType(property, &is_int_property, &need_out_of_mutex);
-  if (!is_int_property) {
+  const DBPropertyInfo* property_info = GetPropertyInfo(property);
+  if (property_info == nullptr || property_info->handle_int == nullptr) {
     return false;
   }
 
@@ -4784,8 +4778,7 @@ bool DBImpl::GetAggregatedIntProperty(const Slice& property,
     InstrumentedMutexLock l(&mutex_);
     uint64_t value;
     for (auto* cfd : *versions_->GetColumnFamilySet()) {
-      if (GetIntPropertyInternal(cfd, property_type, need_out_of_mutex, true,
-                                 &value)) {
+      if (GetIntPropertyInternal(cfd, *property_info, true, &value)) {
         sum += value;
       } else {
         return false;

db/db_impl.h

@@ -893,9 +893,8 @@ class DBImpl : public DB {
                  bool* value_found = nullptr);
 
   bool GetIntPropertyInternal(ColumnFamilyData* cfd,
-                              DBPropertyType property_type,
-                              bool need_out_of_mutex, bool is_locked,
-                              uint64_t* value);
+                              const DBPropertyInfo& property_info,
+                              bool is_locked, uint64_t* value);
 
   bool HasPendingManualCompaction();
   bool HasExclusiveManualCompaction();

db/db_properties_test.cc

@@ -226,6 +226,18 @@ void GetExpectedTableProperties(TableProperties* expected_tp,
 }
 }  // anonymous namespace
 
+TEST_F(DBPropertiesTest, ValidatePropertyInfo) {
+  for (const auto& ppt_name_and_info : InternalStats::ppt_name_to_info) {
+    // If C++ gets a std::string_literal, this would be better to check at
+    // compile-time using static_assert.
+    ASSERT_TRUE(ppt_name_and_info.first.empty() ||
+                !isdigit(ppt_name_and_info.first.back()));
+
+    ASSERT_TRUE((ppt_name_and_info.second.handle_string == nullptr) !=
+                (ppt_name_and_info.second.handle_int == nullptr));
+  }
+}
+
 TEST_F(DBPropertiesTest, AggregatedTableProperties) {
   for (int kTableCount = 40; kTableCount <= 100; kTableCount += 30) {
     const int kKeysPerTable = 100;

db/internal_stats.h

@@ -21,63 +21,29 @@ namespace rocksdb {
 class MemTableList;
 class DBImpl;
 
-// IMPORTANT: If you add a new property here, also add it to the list in
-//            include/rocksdb/db.h
-enum DBPropertyType : uint32_t {
-  kUnknown,
-  kNumFilesAtLevel,  // Number of files at a specific level
-  kLevelStats,       // Return number of files and total sizes of each level
-  kCFStats,          // Return general statitistics of CF
-  kDBStats,          // Return general statitistics of DB
-  kStats,            // Return general statitistics of both DB and CF
-  kSsTables,         // Return a human readable string of current SST files
-  kStartIntTypes,    // ---- Dummy value to indicate the start of integer values
-  kNumImmutableMemTable,         // Return number of immutable mem tables that
-                                 // have not been flushed.
-  kNumImmutableMemTableFlushed,  // Return number of immutable mem tables
-                                 // in memory that have already been flushed
-  kMemtableFlushPending,         // Return 1 if mem table flushing is pending,
-                                 // otherwise 0.
-  kNumRunningFlushes,      // Return the number of currently running flushes.
-  kCompactionPending,      // Return 1 if a compaction is pending. Otherwise 0.
-  kNumRunningCompactions,  // Return the number of currently running
-                           // compactions.
-  kBackgroundErrors,       // Return accumulated background errors encountered.
-  kCurSizeActiveMemTable,  // Return current size of the active memtable
-  kCurSizeAllMemTables,    // Return current size of unflushed
-                           // (active + immutable) memtables
-  kSizeAllMemTables,       // Return current size of all (active + immutable
-                           // + pinned) memtables
-  kNumEntriesInMutableMemtable,    // Return number of deletes in the mutable
-                                   // memtable.
-  kNumEntriesInImmutableMemtable,  // Return sum of number of entries in all
-                                   // the immutable mem tables.
-  kNumDeletesInMutableMemtable,    // Return number of deletion entries in the
-                                   // mutable memtable.
-  kNumDeletesInImmutableMemtable,  // Return the total number of deletion
-                                   // entries in all the immutable mem tables.
-  kEstimatedNumKeys,  // Estimated total number of keys in the database.
-  kEstimatedUsageByTableReaders,  // Estimated memory by table readers.
-  kIsFileDeletionEnabled,         // Equals disable_delete_obsolete_files_,
-                                  // 0 means file deletions enabled
-  kNumSnapshots,                  // Number of snapshots in the system
-  kOldestSnapshotTime,            // Unix timestamp of the first snapshot
-  kNumLiveVersions,
-  kEstimateLiveDataSize,            // Estimated amount of live data in bytes
-  kTotalSstFilesSize,               // Total size of all sst files.
-  kBaseLevel,                       // The level that L0 data is compacted to
-  kEstimatePendingCompactionBytes,  // Estimated bytes to compaction
-  kAggregatedTableProperties,  // Return a string that contains the aggregated
-                               // table properties.
-  kAggregatedTablePropertiesAtLevel,  // Return a string that contains the
-                                      // aggregated
-  // table properties at the specified level.
+// Config for retrieving a property's value.
+struct DBPropertyInfo {
+  bool need_out_of_mutex;
+
+  // gcc had an internal error for initializing union of pointer-to-member-
+  // functions. Workaround is to populate exactly one of the following function
+  // pointers with a non-nullptr value.
+
+  // @param value Value-result argument for storing the property's string value
+  // @param suffix Argument portion of the property. For example, suffix would
+  //      be "5" for the property "rocksdb.num-files-at-level5". So far, only
+  //      certain string properties take an argument.
+  bool (InternalStats::*handle_string)(std::string* value, Slice suffix);
+
+  // @param value Value-result argument for storing the property's uint64 value
+  // @param db Many of the int properties rely on DBImpl methods.
+  // @param version Version is needed in case the property is retrieved without
+  //      holding db mutex, which is only supported for int properties.
+  bool (InternalStats::*handle_int)(uint64_t* value, DBImpl* db,
+                                    Version* version);
 };
 
-extern DBPropertyType GetPropertyType(const Slice& property,
-                                      bool* is_int_property,
-                                      bool* need_out_of_mutex);
-
+extern const DBPropertyInfo* GetPropertyInfo(const Slice& property);
 
 #ifndef ROCKSDB_LITE
 class InternalStats {
@@ -248,14 +214,18 @@ class InternalStats {
 
   uint64_t BumpAndGetBackgroundErrorCount() { return ++bg_error_count_; }
 
-  bool GetStringProperty(DBPropertyType property_type, const Slice& property,
-                         std::string* value);
+  bool GetStringProperty(const DBPropertyInfo& property_info,
+                         const Slice& property, std::string* value);
+
+  bool GetIntProperty(const DBPropertyInfo& property_info, uint64_t* value,
+                      DBImpl* db);
 
-  bool GetIntProperty(DBPropertyType property_type, uint64_t* value,
-                      DBImpl* db) const;
+  bool GetIntPropertyOutOfMutex(const DBPropertyInfo& property_info,
+                                Version* version, uint64_t* value);
 
-  bool GetIntPropertyOutOfMutex(DBPropertyType property_type, Version* version,
-                                uint64_t* value) const;
+  // Store a mapping from the user-facing DB::Properties string to our
+  // DBPropertyInfo struct used internally for retrieving properties.
+  static const std::unordered_map<std::string, DBPropertyInfo> ppt_name_to_info;
 
  private:
   void DumpDBStats(std::string* value);
@@ -321,6 +291,54 @@ class InternalStats {
           seconds_up(0) {}
   } db_stats_snapshot_;
 
+  // Handler functions for getting property values. They use "value" as a value-
+  // result argument, and return true upon successfully setting "value".
+  bool HandleNumFilesAtLevel(std::string* value, Slice suffix);
+  bool HandleLevelStats(std::string* value, Slice suffix);
+  bool HandleStats(std::string* value, Slice suffix);
+  bool HandleCFStats(std::string* value, Slice suffix);
+  bool HandleDBStats(std::string* value, Slice suffix);
+  bool HandleSsTables(std::string* value, Slice suffix);
+  bool HandleAggregatedTableProperties(std::string* value, Slice suffix);
+  bool HandleAggregatedTablePropertiesAtLevel(std::string* value, Slice suffix);
+  bool HandleNumImmutableMemTable(uint64_t* value, DBImpl* db,
+                                  Version* version);
+  bool HandleNumImmutableMemTableFlushed(uint64_t* value, DBImpl* db,
+                                         Version* version);
+  bool HandleMemTableFlushPending(uint64_t* value, DBImpl* db,
+                                  Version* version);
+  bool HandleNumRunningFlushes(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleCompactionPending(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleNumRunningCompactions(uint64_t* value, DBImpl* db,
+                                   Version* version);
+  bool HandleBackgroundErrors(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleCurSizeActiveMemTable(uint64_t* value, DBImpl* db,
+                                   Version* version);
+  bool HandleCurSizeAllMemTables(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleSizeAllMemTables(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleNumEntriesActiveMemTable(uint64_t* value, DBImpl* db,
+                                      Version* version);
+  bool HandleNumEntriesImmMemTables(uint64_t* value, DBImpl* db,
+                                    Version* version);
+  bool HandleNumDeletesActiveMemTable(uint64_t* value, DBImpl* db,
+                                      Version* version);
+  bool HandleNumDeletesImmMemTables(uint64_t* value, DBImpl* db,
+                                    Version* version);
+  bool HandleEstimateNumKeys(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleNumSnapshots(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleOldestSnapshotTime(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleNumLiveVersions(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleIsFileDeletionsEnabled(uint64_t* value, DBImpl* db,
+                                    Version* version);
+  bool HandleBaseLevel(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleTotalSstFilesSize(uint64_t* value, DBImpl* db, Version* version);
+  bool HandleEstimatePendingCompactionBytes(uint64_t* value, DBImpl* db,
+                                            Version* version);
+  bool HandleEstimateTableReadersMem(uint64_t* value, DBImpl* db,
+                                     Version* version);
+  bool HandleEstimateLiveDataSize(uint64_t* value, DBImpl* db,
+                                  Version* version);
+
   // Total number of background errors encountered. Every time a flush task
   // or compaction task fails, this counter is incremented. The failure can
   // be caused by any possible reason, including file system errors, out of
@@ -402,14 +420,20 @@ class InternalStats {
 
   uint64_t BumpAndGetBackgroundErrorCount() { return 0; }
 
-  bool GetStringProperty(DBPropertyType property_type, const Slice& property,
-                         std::string* value) { return false; }
+  bool GetStringProperty(const DBPropertyInfo& property_info,
+                         const Slice& property, std::string* value) {
+    return false;
+  }
 
-  bool GetIntProperty(DBPropertyType property_type, uint64_t* value,
-                      DBImpl* db) const { return false; }
+  bool GetIntProperty(const DBPropertyInfo& property_info, uint64_t* value,
+                      DBImpl* db) const {
+    return false;
+  }
 
-  bool GetIntPropertyOutOfMutex(DBPropertyType property_type, Version* version,
-                                uint64_t* value) const { return false; }
+  bool GetIntPropertyOutOfMutex(const DBPropertyInfo& property_info,
+                                Version* version, uint64_t* value) const {
+    return false;
+  }
 };
 #endif  // !ROCKSDB_LITE
 

include/rocksdb/db.h

@@ -329,6 +329,9 @@ class DB {
 
 #ifndef ROCKSDB_LITE
   // Contains all valid property arguments for GetProperty().
+  //
+  // NOTE: Property names cannot end in numbers since those are interpreted as
+  //       arguments, e.g., see kNumFilesAtLevelPrefix.
   struct Properties {
     //  "rocksdb.num-files-at-level<N>" - returns string containing the number
     //      of files at level <N>, where <N> is an ASCII representation of a