Ensure and clarify how RocksDB calls TablePropertiesCollector's funct…

…ions (#12053)

Summary:
**Context/Summary:**
It's intuitive for users to assume `TablePropertiesCollector::Finish()` is called only once by RocksDB internal by the word "finish".

However, this is currently not true as RocksDB also calls this function in `BlockBased/PlainTableBuilder::GetTableProperties()` to populate user collected properties on demand.

This PR avoids that by moving that populating to where we first call `Finish()` (i.e, `NotifyCollectTableCollectorsOnFinish`)

Bonus: clarified in the API that `GetReadableProperties()` will be called after `Finish()` and added UT to ensure that.

Pull Request resolved: #12053

Test Plan:
- Modified test `DBPropertiesTest.GetUserDefinedTableProperties` to ensure `Finish()` only called once.
- Existing test particularly `db_properties_test, table_properties_collector_test` verify the functionality  `NotifyCollectTableCollectorsOnFinish` and `GetReadableProperties()` are not broken by this change.

Reviewed By: ajkr

Differential Revision: D51095434

Pulled By: hx235

fbshipit-source-id: 1c6275258f9b99dedad313ee8427119126817973

db/db_properties_test.cc

@@ -1084,12 +1084,14 @@ class CountingUserTblPropCollector : public TablePropertiesCollector {
   const char* Name() const override { return "CountingUserTblPropCollector"; }
 
   Status Finish(UserCollectedProperties* properties) override {
+    assert(!finish_called_);
     std::string encoded;
     PutVarint32(&encoded, count_);
     *properties = UserCollectedProperties{
         {"CountingUserTblPropCollector", message_},
         {"Count", encoded},
     };
+    finish_called_ = true;
     return Status::OK();
   }
 
@@ -1101,12 +1103,14 @@ class CountingUserTblPropCollector : public TablePropertiesCollector {
   }
 
   UserCollectedProperties GetReadableProperties() const override {
+    assert(finish_called_);
     return UserCollectedProperties{};
   }
 
  private:
   std::string message_ = "Rocksdb";
   uint32_t count_ = 0;
+  bool finish_called_ = false;
 };
 
 class CountingUserTblPropCollectorFactory

include/rocksdb/table_properties.h

@@ -122,12 +122,15 @@ class TablePropertiesCollector {
 
   // Finish() will be called when a table has already been built and is ready
   // for writing the properties block.
+  // It will be called only once by RocksDB internal.
+  //
   // @params properties  User will add their collected statistics to
   // `properties`.
   virtual Status Finish(UserCollectedProperties* properties) = 0;
 
   // Return the human-readable properties, where the key is property name and
   // the value is the human-readable form of value.
+  // It will only be called after Finish() has been called by RocksDB internal.
   virtual UserCollectedProperties GetReadableProperties() const = 0;
 
   // The name of the properties collector can be used for debugging purpose.

table/block_based/block_based_table_builder.cc

@@ -1710,9 +1710,10 @@ void BlockBasedTableBuilder::WritePropertiesBlock(
     property_block_builder.AddTableProperty(rep_->props);
 
     // Add use collected properties
-    NotifyCollectTableCollectorsOnFinish(rep_->table_properties_collectors,
-                                         rep_->ioptions.logger,
-                                         &property_block_builder);
+    NotifyCollectTableCollectorsOnFinish(
+        rep_->table_properties_collectors, rep_->ioptions.logger,
+        &property_block_builder, rep_->props.user_collected_properties,
+        rep_->props.readable_properties);
 
     Slice block_data = property_block_builder.Finish();
     TEST_SYNC_POINT_CALLBACK(
@@ -2061,14 +2062,7 @@ bool BlockBasedTableBuilder::NeedCompact() const {
 }
 
 TableProperties BlockBasedTableBuilder::GetTableProperties() const {
-  TableProperties ret = rep_->props;
-  for (const auto& collector : rep_->table_properties_collectors) {
-    for (const auto& prop : collector->GetReadableProperties()) {
-      ret.readable_properties.insert(prop);
-    }
-    collector->Finish(&ret.user_collected_properties).PermitUncheckedError();
-  }
-  return ret;
+  return rep_->props;
 }
 
 std::string BlockBasedTableBuilder::GetFileChecksum() const {

table/meta_blocks.cc

@@ -213,21 +213,25 @@ void NotifyCollectTableCollectorsOnBlockAdd(
 
 bool NotifyCollectTableCollectorsOnFinish(
     const std::vector<std::unique_ptr<IntTblPropCollector>>& collectors,
-    Logger* info_log, PropertyBlockBuilder* builder) {
+    Logger* info_log, PropertyBlockBuilder* builder,
+    UserCollectedProperties& user_collected_properties,
+    UserCollectedProperties& readable_properties) {
   bool all_succeeded = true;
   for (auto& collector : collectors) {
-    UserCollectedProperties user_collected_properties;
     Status s = collector->Finish(&user_collected_properties);
-
-    all_succeeded = all_succeeded && s.ok();
-    if (!s.ok()) {
+    if (s.ok()) {
+      for (const auto& prop : collector->GetReadableProperties()) {
+        readable_properties.insert(prop);
+      }
+      builder->Add(user_collected_properties);
+    } else {
       LogPropertiesCollectionError(info_log, "Finish" /* method */,
                                    collector->Name());
-    } else {
-      builder->Add(user_collected_properties);
+      if (all_succeeded) {
+        all_succeeded = false;
+      }
     }
   }
-
   return all_succeeded;
 }
 

table/meta_blocks.h

@@ -98,9 +98,13 @@ void NotifyCollectTableCollectorsOnBlockAdd(
 
 // NotifyCollectTableCollectorsOnFinish() triggers the `Finish` event for all
 // property collectors. The collected properties will be added to `builder`.
+// It will also populate `user_collected_properties` and `readable_properties`
+// with the collected properties.
 bool NotifyCollectTableCollectorsOnFinish(
     const std::vector<std::unique_ptr<IntTblPropCollector>>& collectors,
-    Logger* info_log, PropertyBlockBuilder* builder);
+    Logger* info_log, PropertyBlockBuilder* builder,
+    UserCollectedProperties& user_collected_properties,
+    UserCollectedProperties& readable_properties);
 
 // Read table properties from a file using known BlockHandle.
 // @returns a status to indicate if the operation succeeded. On success,

table/plain/plain_table_builder.cc

@@ -265,12 +265,16 @@ Status PlainTableBuilder::Finish() {
   PropertyBlockBuilder property_block_builder;
   // -- Add basic properties
   property_block_builder.AddTableProperty(properties_);
-
+  // -- Add eixsting user collected properties
   property_block_builder.Add(properties_.user_collected_properties);
-
-  // -- Add user collected properties
+  // -- Add more user collected properties
+  UserCollectedProperties more_user_collected_properties;
   NotifyCollectTableCollectorsOnFinish(
-      table_properties_collectors_, ioptions_.logger, &property_block_builder);
+      table_properties_collectors_, ioptions_.logger, &property_block_builder,
+      more_user_collected_properties, properties_.readable_properties);
+  properties_.user_collected_properties.insert(
+      more_user_collected_properties.begin(),
+      more_user_collected_properties.end());
 
   // -- Write property block
   BlockHandle property_block_handle;

table/plain/plain_table_builder.h

@@ -85,16 +85,7 @@ class PlainTableBuilder : public TableBuilder {
   // Finish() call, returns the size of the final generated file.
   uint64_t FileSize() const override;
 
-  TableProperties GetTableProperties() const override {
-    TableProperties ret = properties_;
-    for (const auto& collector : table_properties_collectors_) {
-      for (const auto& prop : collector->GetReadableProperties()) {
-        ret.readable_properties.insert(prop);
-      }
-      collector->Finish(&ret.user_collected_properties).PermitUncheckedError();
-    }
-    return ret;
-  }
+  TableProperties GetTableProperties() const override { return properties_; }
 
   bool SaveIndexInFile() const { return store_index_in_file_; }
 
@@ -158,4 +149,3 @@ class PlainTableBuilder : public TableBuilder {
 };
 
 }  // namespace ROCKSDB_NAMESPACE
-

unreleased_history/behavior_changes/table_prop_collector_api_call.md

@@ -0,0 +1 @@
+Make RocksDB only call `TablePropertiesCollector::Finish()` once.