finish basic logics

src/config/config.cc

@@ -128,7 +128,7 @@ Config::Config() {
       {"compaction-checker-range", false, new StringField(&compaction_checker_range_, "")},
       {"db-name", true, new StringField(&db_name, "change.me.db")},
       {"dir", true, new StringField(&dir, "/tmp/kvrocks")},
-      {"backup-dir", true, new StringField(&backup_dir, "")},
+      {"backup-dir", false, new StringField(&backup_dir, "")},
       {"log-dir", true, new StringField(&log_dir, "")},
       {"pidfile", true, new StringField(&pidfile, "")},
       {"max-io-mb", false, new IntField(&max_io_mb, 500, 0, INT_MAX)},
@@ -302,7 +302,7 @@ void Config::initFieldValidator() {
 }
 
 // The callback function would be invoked after the field was set,
-// it may change related fileds or re-format the field. for example,
+// it may change related fields or re-format the field. for example,
 // when the 'dir' was set, the db-dir or backup-dir should be reset as well.
 void Config::initFieldCallback() {
   auto set_db_option_cb = [](Server *srv, const std::string &k, const std::string &v) -> Status {
@@ -329,16 +329,30 @@ void Config::initFieldCallback() {
       {"dir",
        [this](Server *srv, const std::string &k, const std::string &v) -> Status {
          db_dir = dir + "/db";
-         srv->storage_->SetBackupDirIfEmpty(dir + "/backup");
+         {
+           std::lock_guard<std::mutex> lg(this->backup_mu_);
+           if (backup_dir.empty()) {
+             backup_dir = dir + "/backup";
+           }
+         }
          if (log_dir.empty()) log_dir = dir;
          checkpoint_dir = dir + "/checkpoint";
          sync_checkpoint_dir = dir + "/sync_checkpoint";
          backup_sync_dir = dir + "/backup_for_sync";
          return Status::OK();
        }},
-      {"backup-dir", [](Server *srv, const std::string &k, const std::string &v) -> Status {
-        srv->storage_->SetBackupDir(v);
-        return Status::OK();
+      {"backup-dir",
+       [this](Server *srv, const std::string &k, const std::string &v) -> Status {
+         std::string previous_backup;
+         {
+           std::lock_guard<std::mutex> lg(this->backup_mu_);
+           previous_backup = std::move(backup_dir);
+           backup_dir = v;
+         }
+         if (!previous_backup.empty()) {
+           LOG(INFO) << "change backup dir from " << backup_dir << " to " << v;
+         }
+         return Status::OK();
        }},
       {"cluster-enabled",
        [this](Server *srv, const std::string &k, const std::string &v) -> Status {

src/config/config.h

@@ -107,7 +107,7 @@ struct Config {
   std::vector<std::string> binds;
   std::string dir;
   std::string db_dir;
-  std::string backup_dir; // GUARD_BY(backup_mu_)
+  std::string backup_dir;  // GUARD_BY(backup_mu_)
   std::string backup_sync_dir;
   std::string checkpoint_dir;
   std::string sync_checkpoint_dir;
@@ -182,6 +182,8 @@ struct Config {
     } write_options;
   } RocksDB;
 
+  mutable std::mutex backup_mu_;
+
  public:
   Status Rewrite();
   Status Load(const std::string &path);

src/storage/storage.cc

@@ -126,22 +126,6 @@ void Storage::SetBlobDB(rocksdb::ColumnFamilyOptions *cf_options) {
   cf_options->blob_garbage_collection_age_cutoff = config_->RocksDB.blob_garbage_collection_age_cutoff / 100.0;
 }
 
-void Storage::SetBackupDir(std::string backup) {
-  // WARN: currently, SetBackupDir may wait for a long time
-  //  when the server is building a backup.
-  std::lock_guard<std::mutex> lg(backup_mu_);
-  config_->backup_dir = std::move(backup);
-}
-
-void Storage::SetBackupDirIfEmpty(std::string backup) {
-  // WARN: currently, SetBackupDir may wait for a long time
-  //  when the server is building a backup.
-  std::lock_guard<std::mutex> lg(backup_mu_);
-  if (config_->backup_dir.empty()) {
-    config_->backup_dir = std::move(backup);
-  }
-}
-
 rocksdb::Options Storage::InitOptions() {
   rocksdb::Options options;
   options.create_if_missing = true;
@@ -349,7 +333,7 @@ Status Storage::OpenForReadOnly() { return Open(true); }
 
 Status Storage::CreateBackup() {
   LOG(INFO) << "[storage] Start to create new backup";
-  std::lock_guard<std::mutex> lg(backup_mu_);
+  std::lock_guard<std::mutex> lg(config_->backup_mu_);
   // Note: `config_->backup_dir` may change, so we should use same dir during the task.
   std::string task_backup_dir = config_->backup_dir;
 
@@ -485,7 +469,7 @@ void Storage::EmptyDB() {
 
 void Storage::PurgeOldBackups(uint32_t num_backups_to_keep, uint32_t backup_max_keep_hours) {
   time_t now = time(nullptr);
-  std::lock_guard<std::mutex> lg(backup_mu_);
+  std::lock_guard<std::mutex> lg(config_->backup_mu_);
   std::string task_backup_dir = config_->backup_dir;
 
   // Return if there is no backup

src/storage/storage.h

@@ -160,13 +160,9 @@ class Storage {
   std::string GetReplIdFromWalBySeq(rocksdb::SequenceNumber seq);
   std::string GetReplIdFromDbEngine(void);
 
-  void SetBackupDir(std::string backup);
-  void SetBackupDirIfEmpty(std::string backup);
-
  private:
   rocksdb::DB *db_ = nullptr;
   std::string replid_;
-  std::mutex backup_mu_;
   time_t backup_creating_time_;
   rocksdb::BackupEngine *backup_ = nullptr;
   rocksdb::Env *env_;

tests/gocase/unit/config/config_test.go

@@ -41,3 +41,26 @@ func TestRenameCommand(t *testing.T) {
 	r := rdb.Do(ctx, "KEYSNEW", "*")
 	require.Equal(t, []interface{}{}, r.Val())
 }
+
+func TestSetConfigBackupDir(t *testing.T) {
+	srv := util.StartServer(t, map[string]string{})
+	defer srv.Close()
+
+	ctx := context.Background()
+	rdb := srv.NewClient()
+	defer func() { require.NoError(t, rdb.Close()) }()
+	r := rdb.Do(ctx, "CONFIG", "GET backup-dir")
+
+	r = rdb.Do(ctx, "bgsave")
+
+	// TODO(mapleFU): os check
+
+	// TODO(mapleFU): set to a proper folder
+	r = rdb.Do(ctx, "CONFIG", "SET backup-dir /tmp")
+
+	r = rdb.Do(ctx, "bgsave")
+
+	// TODO(mapleFU): os check
+
+	require.Equal(t, []interface{}{}, r.Val())
+}