From de3bb39bb8f688421980b0edf72e2e92417bf3f3 Mon Sep 17 00:00:00 2001
From: Hui Xiao <huixiao@fb.com>
Date: Tue, 7 Nov 2023 12:32:55 -0800
Subject: [PATCH] draft

---
 .../block_based/block_based_table_builder.cc  | 15 ++++---------
 table/meta_blocks.cc                          | 21 +++++++++++--------
 table/meta_blocks.h                           |  2 +-
 table/plain/plain_table_builder.cc            |  7 +++----
 table/plain/plain_table_builder.h             | 12 +----------
 5 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc
index cc4f17413b05..bf7d93daad51 100644
--- a/table/block_based/block_based_table_builder.cc
+++ b/table/block_based/block_based_table_builder.cc
@@ -1710,9 +1710,9 @@ 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));
 
     Slice block_data = property_block_builder.Finish();
     TEST_SYNC_POINT_CALLBACK(
@@ -2061,14 +2061,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 {
diff --git a/table/meta_blocks.cc b/table/meta_blocks.cc
index b82b5962f629..735b307313bc 100644
--- a/table/meta_blocks.cc
+++ b/table/meta_blocks.cc
@@ -213,21 +213,24 @@ void NotifyCollectTableCollectorsOnBlockAdd(
 
 bool NotifyCollectTableCollectorsOnFinish(
     const std::vector<std::unique_ptr<IntTblPropCollector>>& collectors,
-    Logger* info_log, PropertyBlockBuilder* builder) {
+    Logger* info_log, PropertyBlockBuilder* builder, TableProperties* props) {
+  assert(props);
   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()) {
+    Status s = collector->Finish(&props->user_collected_properties);
+    if (s.ok()) {
+      for (const auto& prop : collector->GetReadableProperties()) {
+        props->readable_properties.insert(prop);
+      }
+      builder->Add(props->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;
 }
 
diff --git a/table/meta_blocks.h b/table/meta_blocks.h
index 1ed9cf27f16b..9231413b5546 100644
--- a/table/meta_blocks.h
+++ b/table/meta_blocks.h
@@ -100,7 +100,7 @@ void NotifyCollectTableCollectorsOnBlockAdd(
 // property collectors. The collected properties will be added to `builder`.
 bool NotifyCollectTableCollectorsOnFinish(
     const std::vector<std::unique_ptr<IntTblPropCollector>>& collectors,
-    Logger* info_log, PropertyBlockBuilder* builder);
+    Logger* info_log, PropertyBlockBuilder* builder, TableProperties* props);
 
 // Read table properties from a file using known BlockHandle.
 // @returns a status to indicate if the operation succeeded. On success,
diff --git a/table/plain/plain_table_builder.cc b/table/plain/plain_table_builder.cc
index ffa811c3ca83..1d6c11816c54 100644
--- a/table/plain/plain_table_builder.cc
+++ b/table/plain/plain_table_builder.cc
@@ -266,11 +266,10 @@ Status PlainTableBuilder::Finish() {
   // -- Add basic properties
   property_block_builder.AddTableProperty(properties_);
 
-  property_block_builder.Add(properties_.user_collected_properties);
-
   // -- Add user collected properties
-  NotifyCollectTableCollectorsOnFinish(
-      table_properties_collectors_, ioptions_.logger, &property_block_builder);
+  NotifyCollectTableCollectorsOnFinish(table_properties_collectors_,
+                                       ioptions_.logger,
+                                       &property_block_builder, &properties_);
 
   // -- Write property block
   BlockHandle property_block_handle;
diff --git a/table/plain/plain_table_builder.h b/table/plain/plain_table_builder.h
index 27c07898ff0b..fb7ea63be507 100644
--- a/table/plain/plain_table_builder.h
+++ b/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
-