From adc9001f2058c8b6cba2ee72c1ae49dbcb475f86 Mon Sep 17 00:00:00 2001
From: Changyu Bi <changyubi@meta.com>
Date: Fri, 21 Apr 2023 16:54:02 -0700
Subject: [PATCH] Improve error message from `SanityCheckCFOptions()` for
 merge_operator (#11393)

Summary:
This happens when the persisted merge operator not a RocksDB built-in one. This PR improves this error message to include the actual persisted merge operator name. when there is a merge_operator mismatch in `SanityCheckCFOptions()`, for example, going from merge operator "CustomMergeOp" to nullptr, an error message like the following is returned:

"failed the verification on ColumnFamilyOptions::merge_operator--- The specified one is nullptr while the **persisted one is nullptr**."

This happens when the persisted merge operator not a RocksDB built-in one. This PR improves this error message to include the actual persisted merge operator name.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11393

Test Plan: add unit test to check error message when going from merge op -> nullptr and going from merge op1 to merge op 2.

Reviewed By: ajkr

Differential Revision: D45190131

Pulled By: cbi42

fbshipit-source-id: 67712c2fec29c654c15166d1be985e710e6081e5
---
 options/options_parser.cc |  9 +++++++++
 options/options_test.cc   | 30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/options/options_parser.cc b/options/options_parser.cc
index b3754de79..d423f76ff 100644
--- a/options/options_parser.cc
+++ b/options/options_parser.cc
@@ -679,6 +679,15 @@ Status RocksDBOptionsParser::VerifyCFOptions(
     Status s = base_config->GetOption(config_options, mismatch, &base_value);
     if (s.ok()) {
       s = file_config->GetOption(config_options, mismatch, &file_value);
+      // In file_opt, certain options like MergeOperator may be nullptr due to
+      //   factor methods not available. So we use opt_map to get
+      //   option value to use in the error message below.
+      if (s.ok() && file_value == kNullptrString && opt_map) {
+        auto const& opt_val_str = (opt_map->find(mismatch));
+        if (opt_val_str != opt_map->end()) {
+          file_value = opt_val_str->second;
+        }
+      }
     }
     int offset = snprintf(buffer, sizeof(buffer),
                           "[RocksDBOptionsParser]: "
diff --git a/options/options_test.cc b/options/options_test.cc
index 556815745..c22df98c6 100644
--- a/options/options_test.cc
+++ b/options/options_test.cc
@@ -3927,6 +3927,36 @@ class OptionsSanityCheckTest : public OptionsParserTest,
   const std::string kOptionsFileName = "OPTIONS";
 };
 
+TEST_P(OptionsSanityCheckTest, MergeOperatorErrorMessage) {
+  ColumnFamilyOptions opts;
+  Random rnd(301);
+  opts.merge_operator.reset(test::RandomMergeOperator(&rnd));
+  std::string merge_op_name = opts.merge_operator->Name();
+  ASSERT_OK(PersistCFOptions(opts));
+
+  // Test when going from merge operator -> nullptr
+  opts.merge_operator = nullptr;
+  Status s =
+      SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelLooselyCompatible);
+  ASSERT_TRUE(s.IsInvalidArgument());
+  std::string err_msg = s.ToString();
+  std::string specified = "The specified one is " + kNullptrString;
+  std::string persisted = "the persisted one is " + merge_op_name;
+  ASSERT_TRUE(err_msg.find(specified) != std::string::npos);
+  ASSERT_TRUE(err_msg.find(persisted) != std::string::npos);
+
+  // Test when using a different merge operator
+  opts.merge_operator.reset(test::RandomMergeOperator(&rnd));
+  s = SanityCheckCFOptions(opts, ConfigOptions::kSanityLevelLooselyCompatible);
+  ASSERT_TRUE(s.IsInvalidArgument());
+  err_msg = s.ToString();
+  specified =
+      "The specified one is " + std::string(opts.merge_operator->Name());
+  persisted = "the persisted one is " + merge_op_name;
+  ASSERT_TRUE(err_msg.find(specified) != std::string::npos);
+  ASSERT_TRUE(err_msg.find(persisted) != std::string::npos);
+}
+
 TEST_P(OptionsSanityCheckTest, CFOptionsSanityCheck) {
   ColumnFamilyOptions opts;
   Random rnd(301);