Add validation check for self-driving knob actions and add dynamic setting to wal_serialization_interval (#1438)

linmagit · web-flow · commit 0c17df0de6e8 · 2021-01-18T22:18:46.000-05:00
diff --git a/src/include/self_driving/pilot/action/abstract_action.h b/src/include/self_driving/pilot/action/abstract_action.h
@@ -68,6 +68,15 @@ class AbstractAction {
    */
   virtual const std::string &GetSQLCommand() { return sql_command_; }
 
+  /**
+   * Check whether the action is valid to apply.
+   * Possible scenarios that the action is invalid to apply: the knob setting is out of the valid range, the index
+   * requires more memory than available in the system, etc.
+   * TODO(lin): add the available memory as input param
+   * @return true if the action is valid to apply, false otherwise
+   */
+  virtual bool IsValid() { return true; }
+
  protected:
   std::string sql_command_;  ///< The SQL commaned used to apply the action
 
diff --git a/src/include/self_driving/pilot/action/change_knob_action.h b/src/include/self_driving/pilot/action/change_knob_action.h
@@ -26,19 +26,18 @@ class ChangeKnobAction : public AbstractAction {
    * @param settings_manager SettingsManager (used to find our the current knob value before applying the change_value)
    */
   ChangeKnobAction(settings::Param param, std::string param_name, T change_value,
-                   common::ManagedPointer<settings::SettingsManager> settings_manager)
-      : AbstractAction(ActionType::CHANGE_KNOB),
-        param_(param),
-        param_name_(std::move(param_name)),
-        change_value_(change_value),
-        settings_manager_(settings_manager) {}
+                   common::ManagedPointer<settings::SettingsManager> settings_manager);
 
   const std::string &GetSQLCommand() override;
 
+  bool IsValid() override;
+
  private:
   settings::Param param_;
   std::string param_name_;
   T change_value_;
+  T param_min_value_;
+  T param_max_value_;
   common::ManagedPointer<settings::SettingsManager> settings_manager_;
 };
 
diff --git a/src/include/self_driving/pilot/action/change_knob_value_config.h b/src/include/self_driving/pilot/action/change_knob_value_config.h
@@ -22,6 +22,12 @@ class ChangeKnobValueConfig {
     return common::ManagedPointer(&bool_change_value_map);
   }
 
+  /** @return Map from a int knob param to the vector of its candidate change values */
+  static common::ManagedPointer<std::map<settings::Param, std::vector<std::pair<int32_t, int32_t>>>>
+  GetIntChangeValueMap() {
+    return common::ManagedPointer(&int_change_value_map);
+  }
+
   /** @return Map from a int64 knob param to the vector of its candidate change values */
   static common::ManagedPointer<std::map<settings::Param, std::vector<std::pair<int64_t, int64_t>>>>
   GetInt64ChangeValueMap() {
@@ -30,6 +36,7 @@ class ChangeKnobValueConfig {
 
  private:
   static std::map<settings::Param, std::vector<std::pair<bool, bool>>> bool_change_value_map;
+  static std::map<settings::Param, std::vector<std::pair<int32_t, int32_t>>> int_change_value_map;
   static std::map<settings::Param, std::vector<std::pair<int64_t, int64_t>>> int64_change_value_map;
 };
 
diff --git a/src/include/settings/settings_callbacks.h b/src/include/settings/settings_callbacks.h
@@ -78,6 +78,16 @@ class Callbacks {
   static void WalNumBuffers(void *old_value, void *new_value, DBMain *db_main,
                             common::ManagedPointer<common::ActionContext> action_context);
 
+  /**
+   * Changes the number of buffers the log manager uses.
+   * @param old_value old settings value
+   * @param new_value new settings value
+   * @param db_main pointer to db_main
+   * @param action_context pointer to the action context for this settings change
+   */
+  static void WalSerializationInterval(void *old_value, void *new_value, DBMain *db_main,
+                                       common::ManagedPointer<common::ActionContext> action_context);
+
   /**
    * Enable or disable metrics collection for Logging component
    * @param old_value old settings value
diff --git a/src/include/settings/settings_defs.h b/src/include/settings/settings_defs.h
@@ -125,8 +125,8 @@ SETTING_int(
     100,
     1,
     10000,
-    false,
-    noisepage::settings::Callbacks::NoOp
+    true,
+    noisepage::settings::Callbacks::WalSerializationInterval
 )
 
 // Log file persisting interval
diff --git a/src/include/settings/settings_param.h b/src/include/settings/settings_param.h
@@ -13,6 +13,8 @@ class DBMain;
 
 namespace noisepage::selfdriving::pilot {
 class ChangeKnobActionGenerator;
+template <class T>
+class ChangeKnobAction;
 namespace test {
 class GenerateChangeKnobAction_GenerateAction_Test;
 }
@@ -75,6 +77,9 @@ class ParamInfo {
   friend class noisepage::DBMain;
   friend class SettingsManager;
   friend class selfdriving::pilot::ChangeKnobActionGenerator;
+  friend class selfdriving::pilot::ChangeKnobAction<bool>;
+  friend class selfdriving::pilot::ChangeKnobAction<int32_t>;
+  friend class selfdriving::pilot::ChangeKnobAction<int64_t>;
   friend class selfdriving::pilot::test::GenerateChangeKnobAction_GenerateAction_Test;
   std::string name_;
   parser::ConstantValueExpression value_;
diff --git a/src/include/storage/write_ahead_log/log_manager.h b/src/include/storage/write_ahead_log/log_manager.h
@@ -126,6 +126,15 @@ class LogManager : public common::DedicatedThreadOwner {
     return false;
   }
 
+  /**
+   * Set the new log serialization interval in microseconds.
+   * @param interval the new serialization interval in microseconds (should > 0)
+   */
+  void SetSerializationInterval(int32_t interval);
+
+  /** @return the log serialization interval */
+  int32_t GetSerializationInterval() { return serialization_interval_.count(); }
+
  private:
   // Flag to tell us when the log manager is running or during termination
   bool run_log_manager_;
@@ -151,7 +160,7 @@ class LogManager : public common::DedicatedThreadOwner {
   // Log serializer task that processes buffers handed over by transactions and serializes them into consumer buffers
   common::ManagedPointer<LogSerializerTask> log_serializer_task_ = common::ManagedPointer<LogSerializerTask>(nullptr);
   // Interval used by log serialization task
-  const std::chrono::microseconds serialization_interval_;
+  std::chrono::microseconds serialization_interval_;
 
   // The log consumer task which flushes filled buffers to the disk
   common::ManagedPointer<DiskLogConsumerTask> disk_log_writer_task_ =
diff --git a/src/include/storage/write_ahead_log/log_serializer_task.h b/src/include/storage/write_ahead_log/log_serializer_task.h
@@ -74,12 +74,18 @@ class LogSerializerTask : public common::DedicatedThreadTask {
     }
   }
 
+  /**
+   * Set the new log serialization interval in microseconds.
+   * @param interval the new serialization interval in microseconds (should > 0)
+   */
+  void SetSerializationInterval(int32_t interval) { serialization_interval_ = std::chrono::microseconds(interval); }
+
  private:
   friend class LogManager;
   // Flag to signal task to run or stop
   bool run_task_;
   // Interval for serialization
-  const std::chrono::microseconds serialization_interval_;
+  std::chrono::microseconds serialization_interval_;
 
   // Used to release processed buffers
   RecordBufferSegmentPool *buffer_pool_;
diff --git a/src/self_driving/pilot/action/change_knob_action.cpp b/src/self_driving/pilot/action/change_knob_action.cpp
@@ -5,9 +5,22 @@
 
 namespace noisepage::selfdriving::pilot {
 
+template <class T>
+ChangeKnobAction<T>::ChangeKnobAction(settings::Param param, std::string param_name, T change_value,
+                                      common::ManagedPointer<settings::SettingsManager> settings_manager)
+    : AbstractAction(ActionType::CHANGE_KNOB),
+      param_(param),
+      param_name_(std::move(param_name)),
+      change_value_(change_value),
+      settings_manager_(settings_manager) {
+  auto param_info = settings_manager_->GetParamInfo(param_);
+  param_min_value_ = param_info.min_value_;
+  param_max_value_ = param_info.max_value_;
+}
+
 template <class T>
 const std::string &ChangeKnobAction<T>::GetSQLCommand() {
-  sql_command_ = "set " + param_name_ + " ";
+  sql_command_ = "set " + param_name_ + " = ";
   // Set the new value accordingly based on the param type
   if constexpr (std::is_same<T, bool>::value) {  // NOLINT
     T original_value = settings_manager_->GetBool(param_);
@@ -22,12 +35,30 @@ const std::string &ChangeKnobAction<T>::GetSQLCommand() {
     T new_value = original_value + change_value_;
     sql_command_ += std::to_string(new_value) + ";";
   } else {
-    throw PILOT_EXCEPTION(fmt::format("Unexpected knob parameter type\"{}\"", param_name_),
+    throw PILOT_EXCEPTION(fmt::format("Unexpected knob parameter type \"{}\"", param_name_),
                           common::ErrorCode::ERRCODE_INTERNAL_ERROR);
   }
   return sql_command_;
 }
 
+template <class T>
+bool ChangeKnobAction<T>::IsValid() {
+  if constexpr (std::is_same<T, bool>::value) {  // NOLINT
+    return true;
+  } else if constexpr (std::is_same<T, int32_t>::value) {  // NOLINT
+    T original_value = settings_manager_->GetInt(param_);
+    T new_value = original_value + change_value_;
+    return new_value <= param_max_value_ && new_value >= param_min_value_;
+  } else if constexpr (std::is_same<T, int64_t>::value) {  // NOLINT
+    T original_value = settings_manager_->GetInt64(param_);
+    T new_value = original_value + change_value_;
+    return new_value <= param_max_value_ && new_value >= param_min_value_;
+  } else {
+    throw PILOT_EXCEPTION(fmt::format("Unexpected knob parameter type \"{}\"", param_name_),
+                          common::ErrorCode::ERRCODE_INTERNAL_ERROR);
+  }
+}
+
 template class ChangeKnobAction<bool>;
 template class ChangeKnobAction<int32_t>;
 template class ChangeKnobAction<int64_t>;
diff --git a/src/self_driving/pilot/action/change_knob_value_config.cpp b/src/self_driving/pilot/action/change_knob_value_config.cpp
@@ -7,8 +7,10 @@ std::map<settings::Param, std::vector<std::pair<bool, bool>>> ChangeKnobValueCon
     {settings::Param::compiled_query_execution, {{true, true}}},
 };
 
-std::map<settings::Param, std::vector<std::pair<int64_t, int64_t>>> ChangeKnobValueConfig::int64_change_value_map = {
-    {settings::Param::wal_persist_threshold, {{1 << 10, -(1 << 10)}, {1 << 12, -(1 << 12)}}},
+std::map<settings::Param, std::vector<std::pair<int32_t, int32_t>>> ChangeKnobValueConfig::int_change_value_map = {
+    {settings::Param::wal_serialization_interval, {{10, -10}, {100, -100}}},
 };
 
+std::map<settings::Param, std::vector<std::pair<int64_t, int64_t>>> ChangeKnobValueConfig::int64_change_value_map = {};
+
 }  // namespace noisepage::selfdriving::pilot
diff --git a/src/self_driving/pilot/action/generators/change_knob_action_generator.cpp b/src/self_driving/pilot/action/generators/change_knob_action_generator.cpp
@@ -14,6 +14,7 @@ void ChangeKnobActionGenerator::GenerateActions(const std::vector<std::unique_pt
                                                 std::vector<action_id_t> *candidate_actions) {
   NOISEPAGE_ASSERT(settings_manager != nullptr, "No SettingsManager provided!");
   GenerateActionForType<bool>(settings_manager, action_map, candidate_actions);
+  GenerateActionForType<int32_t>(settings_manager, action_map, candidate_actions);
   GenerateActionForType<int64_t>(settings_manager, action_map, candidate_actions);
 }
 
@@ -24,6 +25,8 @@ void ChangeKnobActionGenerator::GenerateActionForType(
   common::ManagedPointer<std::map<settings::Param, std::vector<std::pair<T, T>>>> knob_change_value_map = nullptr;
   if constexpr (std::is_same<T, bool>::value) {  // NOLINT
     knob_change_value_map = ChangeKnobValueConfig::GetBoolChangeValueMap();
+  } else if constexpr (std::is_same<T, int32_t>::value) {  // NOLINT
+    knob_change_value_map = ChangeKnobValueConfig::GetIntChangeValueMap();
   } else if constexpr (std::is_same<T, int64_t>::value) {  // NOLINT
     knob_change_value_map = ChangeKnobValueConfig::GetInt64ChangeValueMap();
   } else {
diff --git a/src/settings/settings_callbacks.cpp b/src/settings/settings_callbacks.cpp
@@ -60,6 +60,14 @@ void Callbacks::WalNumBuffers(void *const old_value, void *const new_value, DBMa
     action_context->SetState(common::ActionState::FAILURE);
 }
 
+void Callbacks::WalSerializationInterval(void *const old_value, void *const new_value, DBMain *const db_main,
+                                         common::ManagedPointer<common::ActionContext> action_context) {
+  action_context->SetState(common::ActionState::IN_PROGRESS);
+  int new_interval = *static_cast<int *>(new_value);
+  db_main->GetLogManager()->SetSerializationInterval(new_interval);
+  action_context->SetState(common::ActionState::SUCCESS);
+}
+
 void Callbacks::MetricsLogging(void *const old_value, void *const new_value, DBMain *const db_main,
                                common::ManagedPointer<common::ActionContext> action_context) {
   action_context->SetState(common::ActionState::IN_PROGRESS);
diff --git a/src/storage/write_ahead_log/log_manager.cpp b/src/storage/write_ahead_log/log_manager.cpp
@@ -72,4 +72,10 @@ void LogManager::AddBufferToFlushQueue(RecordBufferSegment *const buffer_segment
   log_serializer_task_->AddBufferToFlushQueue(buffer_segment);
 }
 
+void LogManager::SetSerializationInterval(int32_t interval) {
+  NOISEPAGE_ASSERT(interval > 0, "Log serialization interval should be greater than 0");
+  serialization_interval_ = std::chrono::microseconds(interval);
+  if (log_serializer_task_ != nullptr) log_serializer_task_->SetSerializationInterval(interval);
+}
+
 }  // namespace noisepage::storage
diff --git a/test/self_driving/generate_change_knob_action_test.cpp b/test/self_driving/generate_change_knob_action_test.cpp
@@ -29,43 +29,77 @@ TEST_F(GenerateChangeKnobAction, GenerateAction) {
 
   // Each bool knob only has on action since the action is self-reverse
   auto bool_change_value_map = ChangeKnobValueConfig::GetBoolChangeValueMap();
+  auto int_change_value_map = ChangeKnobValueConfig::GetIntChangeValueMap();
   auto int64_change_value_map = ChangeKnobValueConfig::GetInt64ChangeValueMap();
   size_t num_actions = bool_change_value_map->size();
+  for (auto &it : *int_change_value_map) num_actions += it.second.size() * 2;
   for (auto &it : *int64_change_value_map) num_actions += it.second.size() * 2;
 
   EXPECT_EQ(action_map.size(), num_actions);
   EXPECT_EQ(candidate_actions.size(), num_actions);
 
+  size_t num_valid_actions = 0;
   std::unordered_set<std::string> commands;
   commands.reserve(action_map.size());
   for (auto &it : action_map) {
     commands.emplace(it.second->GetSQLCommand());
+    num_valid_actions += it.second->IsValid() ? 1 : 0;
   }
 
+  size_t expected_num_valid_actions = 0;
+
   std::unordered_set<std::string> expected_commands;
   expected_commands.reserve(action_map.size());
   for (auto &it : *bool_change_value_map) {
     auto param = it.first;
     bool original_value = settings_manager->GetBool(param);
-    std::string command = "set " + settings_manager->GetParamInfo(param).name_;
-    command += original_value ? " 'false';" : " 'true';";
+    std::string command = "set " + settings_manager->GetParamInfo(param).name_ + " = ";
+    command += original_value ? "'false';" : "'true';";
     expected_commands.emplace(command);
+    // Change bool actions should always be valid
+    expected_num_valid_actions++;
+  }
+
+  for (auto &it : *int_change_value_map) {
+    auto param = it.first;
+    int32_t original_value = settings_manager->GetInt(param);
+
+    for (auto &value_pair : it.second) {
+      auto param_info = settings_manager->GetParamInfo(param);
+      std::string command = "set " + param_info.name_ + " = ";
+      int new_value = original_value + value_pair.first;
+      expected_num_valid_actions += new_value <= param_info.max_value_ && new_value >= param_info.min_value_ ? 1 : 0;
+      command += std::to_string(new_value) + ";";
+      expected_commands.emplace(command);
+      command = "set " + param_info.name_ + " = ";
+      new_value = original_value + value_pair.second;
+      expected_num_valid_actions += new_value <= param_info.max_value_ && new_value >= param_info.min_value_ ? 1 : 0;
+      command += std::to_string(new_value) + ";";
+      expected_commands.emplace(command);
+    }
   }
+
   for (auto &it : *int64_change_value_map) {
     auto param = it.first;
     int64_t original_value = settings_manager->GetInt64(param);
 
     for (auto &value_pair : it.second) {
-      std::string command = "set " + settings_manager->GetParamInfo(param).name_;
-      command += " " + std::to_string(original_value + value_pair.first) + ";";
+      auto param_info = settings_manager->GetParamInfo(param);
+      std::string command = "set " + param_info.name_ + " = ";
+      int64_t new_value = original_value + value_pair.first;
+      expected_num_valid_actions += new_value <= param_info.max_value_ && new_value >= param_info.min_value_ ? 1 : 0;
+      command += std::to_string(new_value) + ";";
       expected_commands.emplace(command);
-      command = "set " + settings_manager->GetParamInfo(param).name_;
-      command += " " + std::to_string(original_value + value_pair.second) + ";";
+      command = "set " + param_info.name_ + " = ";
+      new_value = original_value + value_pair.second;
+      expected_num_valid_actions += new_value <= param_info.max_value_ && new_value >= param_info.min_value_ ? 1 : 0;
+      command += std::to_string(new_value) + ";";
       expected_commands.emplace(command);
     }
   }
 
   EXPECT_EQ(commands, expected_commands);
+  EXPECT_EQ(expected_num_valid_actions, expected_num_valid_actions);
 }
 
 }  // namespace noisepage::selfdriving::pilot::test
diff --git a/test/settings/settings_test.cpp b/test/settings/settings_test.cpp
@@ -178,6 +178,7 @@ TEST_F(SettingsTests, ParamCallbackTest) {
 
 // NOLINTNEXTLINE
 TEST_F(SettingsTests, LogManagerSettingsTest) {
+  // Test the wal_num_buffers setting
   const common::action_id_t action_id(1);
 
   // Check default value is correctly passed to log manager
@@ -195,6 +196,25 @@ TEST_F(SettingsTests, LogManagerSettingsTest) {
   // Check new value is propagated
   EXPECT_EQ(new_num_buffers, settings_manager_->GetInt64(Param::wal_num_buffers));
   EXPECT_EQ(new_num_buffers, log_manager_->TestGetNumBuffers());
+
+  // Test the wal_serialization_interval setting
+  const common::action_id_t action_id2(2);
+
+  // Check default value is correctly passed to log manager
+  auto serializatio_interval = settings_manager_->GetInt64(Param::wal_serialization_interval);
+  EXPECT_EQ(serializatio_interval, log_manager_->GetSerializationInterval());
+
+  // Change value
+  auto new_serializatio_interval = serializatio_interval + 1;
+
+  auto action_context2 = std::make_unique<common::ActionContext>(action_id2);
+  setter_callback_fn setter_callback2 = SettingsTests::EmptySetterCallback;
+  settings_manager_->SetInt(Param::wal_serialization_interval, new_serializatio_interval,
+                            common::ManagedPointer(action_context2), setter_callback2);
+
+  // Check new value is propagated
+  EXPECT_EQ(new_serializatio_interval, settings_manager_->GetInt(Param::wal_serialization_interval));
+  EXPECT_EQ(new_serializatio_interval, log_manager_->GetSerializationInterval());
 }
 
 // Test concurrent modification to buffer pool size.
