Address comments for 'Apply sst file manager in myrocks clone' (#1388)

Summary:
This change is to address the comments of Apply sst file manager in myrocks clone to add slow-rm during checkpoints removal (#1386). Will squash after commit.

Pull Request resolved: #1388

Differential Revision: D51107316

fbshipit-source-id: 69903b786972f8754afac7f3d8697d162266d34f

Author: sunshine-Chun

storage/rocksdb/clone/donor.cc

@@ -56,8 +56,8 @@ namespace {
 class [[nodiscard]] rdb_checkpoint final {
  public:
   rdb_checkpoint()
-      : m_dir{
-            make_dir_name(m_next_id.fetch_add(1, std::memory_order_relaxed))} {}
+      : m_prefix_dir{make_dir_prefix_name(
+            m_next_id.fetch_add(1, std::memory_order_relaxed))} {}
 
   ~rdb_checkpoint() {
     // Ignore the return value - at this point the clone operation is completing
@@ -68,9 +68,8 @@ class [[nodiscard]] rdb_checkpoint final {
   // Returns MySQL error code
   [[nodiscard]] int init() {
     assert(!m_active);
-    m_full_dir = make_dir_full_name(
-        m_dir.c_str(), m_next_sub_id.fetch_add(1, std::memory_order_relaxed));
-    const auto result = myrocks::rocksdb_create_checkpoint(m_full_dir.c_str());
+    m_dir = make_dir_name(m_prefix_dir, m_next_sub_id++);
+    const auto result = myrocks::rocksdb_create_checkpoint(m_dir.c_str());
     m_active = (result == HA_EXIT_SUCCESS);
     return m_active ? 0 : ER_INTERNAL_ERROR;
   }
@@ -79,22 +78,21 @@ class [[nodiscard]] rdb_checkpoint final {
   [[nodiscard]] int cleanup() {
     if (!m_active) return 0;
     m_active = false;
-    return myrocks::rocksdb_remove_checkpoint(m_full_dir.c_str()) ==
-                   HA_EXIT_SUCCESS
+    return myrocks::rocksdb_remove_checkpoint(m_dir.c_str()) == HA_EXIT_SUCCESS
                ? 0
                : ER_INTERNAL_ERROR;
   }
 
   [[nodiscard]] constexpr const std::string &get_dir() const noexcept {
     assert(m_active);
-    return m_full_dir;
+    return m_dir;
   }
 
   [[nodiscard]] std::string path(const std::string &file_name) const {
     // We might be calling this for inactive checkpoint too, if the donor is in
     // the middle of a checkpoint roll. The caller will handle any ENOENTs as
     // needed.
-    return myrocks::rdb_concat_paths(m_full_dir, file_name);
+    return myrocks::rdb_concat_paths(m_dir, file_name);
   }
 
   rdb_checkpoint(const rdb_checkpoint &) = delete;
@@ -103,17 +101,17 @@ class [[nodiscard]] rdb_checkpoint final {
   rdb_checkpoint &operator=(rdb_checkpoint &&) = delete;
 
  private:
-  const std::string m_dir;
+  const std::string m_prefix_dir;
 
-  std::string m_full_dir;
+  std::string m_dir;
 
   bool m_active = false;
 
   static std::atomic<std::uint64_t> m_next_id;
 
-  std::atomic<std::uint64_t> m_next_sub_id{1};
+  std::uint64_t m_next_sub_id = 1;
 
-  [[nodiscard]] static std::string make_dir_name(std::uint64_t id) {
+  [[nodiscard]] static std::string make_dir_prefix_name(std::uint64_t id) {
     const auto base_str = myrocks::clone::checkpoint_base_dir();
     const auto id_str = std::to_string(id);
     std::string result;
@@ -128,14 +126,14 @@ class [[nodiscard]] rdb_checkpoint final {
     return result;
   }
 
-  [[nodiscard]] static std::string make_dir_full_name(std::string base_dir,
-                                                      std::uint64_t id) {
+  [[nodiscard]] static std::string make_dir_name(
+      const std::string &dir_name_prefix, std::uint64_t id) {
     const auto id_str = std::to_string(id);
     std::string result;
-    result.reserve(base_dir.length() + id_str.length() +
+    result.reserve(dir_name_prefix.length() + id_str.length() +
                    1);  // +1 for '-', the trailing
                         // '\0' is accounted by the sizeof.
-    result = base_dir;
+    result = dir_name_prefix;
     result += '-';
     result += id_str;
     return result;

storage/rocksdb/ha_rocksdb.cc

@@ -1290,6 +1290,26 @@ static void rocksdb_set_reset_stats(
   RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex);
 }
 
+int rocksdb_remove_checkpoint(const char *checkpoint_dir_raw) {
+  const auto checkpoint_dir = rdb_normalize_dir(checkpoint_dir_raw);
+  LogPluginErrMsg(INFORMATION_LEVEL, ER_LOG_PRINTF_MSG,
+                  "deleting temporary checkpoint in directory : %s\n",
+                  checkpoint_dir.c_str());
+
+  auto op = rocksdb::Options();
+  op.sst_file_manager.reset(NewSstFileManager(
+      rocksdb_db_options->env, rocksdb_db_options->info_log, "",
+      rocksdb_sst_mgr_rate_bytes_per_sec, false /* delete_existing_trash */));
+  const auto status = rocksdb::DestroyDB(checkpoint_dir, op);
+
+  if (status.ok()) {
+    return HA_EXIT_SUCCESS;
+  }
+  my_error(ER_GET_ERRMSG, MYF(0), status.code(), status.ToString().c_str(),
+           rocksdb_hton_name);
+  return HA_EXIT_FAILURE;
+}
+
 #ifndef __APPLE__
 
 static void rocksdb_set_io_write_timeout(
@@ -1310,27 +1330,6 @@ static void rocksdb_set_io_write_timeout(
   RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex);
 }
 
-int rocksdb_remove_checkpoint(const char *checkpoint_dir_raw) {
-  const auto checkpoint_dir = rdb_normalize_dir(checkpoint_dir_raw);
-  LogPluginErrMsg(INFORMATION_LEVEL, ER_LOG_PRINTF_MSG,
-                  "deleting temporary checkpoint in directory : %s\n",
-                  checkpoint_dir.c_str());
-
-  std::string trash_dir = std::string(rocksdb_datadir) + "/trash";
-  rocksdb::Options op = rocksdb::Options();
-  op.sst_file_manager.reset(NewSstFileManager(
-      rocksdb_db_options->env, rocksdb_db_options->info_log, trash_dir,
-      rocksdb_sst_mgr_rate_bytes_per_sec, true /* delete_existing_trash */));
-  const auto status = rocksdb::DestroyDB(checkpoint_dir, op);
-
-  if (status.ok()) {
-    return HA_EXIT_SUCCESS;
-  }
-  my_error(ER_GET_ERRMSG, MYF(0), status.code(), status.ToString().c_str(),
-           rocksdb_hton_name);
-  return HA_EXIT_FAILURE;
-}
-
 #endif  // !__APPLE__
 
 enum rocksdb_flush_log_at_trx_commit_type : unsigned int {