Fix the pidfile and backup_dir will be rewritten even though they weren't modified

src/cli/main.cc

@@ -154,12 +154,12 @@ int main(int argc, char *argv[]) {
   }
   bool is_supervised = IsSupervisedMode(config.supervised_mode);
   if (config.daemonize && !is_supervised) Daemonize();
-  s = CreatePidFile(config.GetPidFile());
+  s = CreatePidFile(config.pidfile);
   if (!s.IsOK()) {
     LOG(ERROR) << "Failed to create pidfile: " << s.Msg();
     return 1;
   }
-  auto pidfile_exit = MakeScopeExit([&config] { RemovePidFile(config.GetPidFile()); });
+  auto pidfile_exit = MakeScopeExit([&config] { RemovePidFile(config.pidfile); });
 
 #ifdef ENABLE_OPENSSL
   // initialize OpenSSL

src/config/config.cc

@@ -42,6 +42,9 @@
 #include "status.h"
 #include "storage/redis_metadata.h"
 
+constexpr const char *kDefaultDir = "/tmp/kvrocks";
+constexpr const char *kDefaultBackupDir = "/tmp/kvrocks/backup";
+constexpr const char *kDefaultPidfile = "/tmp/kvrocks/kvrocks.pid";
 constexpr const char *kDefaultBindAddress = "127.0.0.1";
 
 constexpr const char *errBlobDbNotEnabled = "Must set rocksdb.enable_blob_files to yes first.";
@@ -141,11 +144,11 @@ Config::Config() {
       {"force-compact-file-min-deleted-percentage", false,
        new IntField(&force_compact_file_min_deleted_percentage, 10, 1, 100)},
       {"db-name", true, new StringField(&db_name, "change.me.db")},
-      {"dir", true, new StringField(&dir, "/tmp/kvrocks")},
-      {"backup-dir", false, new StringField(&backup_dir_, "")},
+      {"dir", true, new StringField(&dir, kDefaultDir)},
+      {"backup-dir", false, new StringField(&backup_dir, kDefaultBackupDir)},
       {"log-dir", true, new StringField(&log_dir, "")},
       {"log-level", false, new EnumField<int>(&log_level, log_levels, google::INFO)},
-      {"pidfile", true, new StringField(&pidfile_, "")},
+      {"pidfile", true, new StringField(&pidfile, kDefaultPidfile)},
       {"max-io-mb", false, new IntField(&max_io_mb, 0, 0, INT_MAX)},
       {"max-bitmap-to-string-mb", false, new IntField(&max_bitmap_to_string_mb, 16, 0, INT_MAX)},
       {"max-db-size", false, new IntField(&max_db_size, 0, 0, INT_MAX)},
@@ -403,6 +406,8 @@ void Config::initFieldCallback() {
              checkpoint_dir = dir + "/checkpoint";
              sync_checkpoint_dir = dir + "/sync_checkpoint";
              backup_sync_dir = dir + "/backup_for_sync";
+             if (backup_dir == kDefaultBackupDir) backup_dir = dir + "/backup";
+             if (pidfile == kDefaultPidfile) pidfile = dir + "/kvrocks.pid";
              return Status::OK();
            }},
           {"backup-dir",
@@ -412,8 +417,8 @@ void Config::initFieldCallback() {
                // Note: currently, backup_mu_ may block by backing up or purging,
                //  the command may wait for seconds.
                std::lock_guard<std::mutex> lg(this->backup_mu);
-               previous_backup = std::move(backup_dir_);
-               backup_dir_ = v;
+               previous_backup = std::move(backup_dir);
+               backup_dir = v;
              }
              if (!previous_backup.empty() && srv != nullptr && !srv->IsLoading()) {
                // LOG(INFO) should be called after log is initialized and server is loaded.
@@ -778,9 +783,7 @@ Status Config::finish() {
   if (master_port != 0 && binds.size() == 0) {
     return {Status::NotOK, "replication doesn't support unix socket"};
   }
-  if (backup_dir_.empty()) backup_dir_ = dir + "/backup";
   if (db_dir.empty()) db_dir = dir + "/db";
-  if (pidfile_.empty()) pidfile_ = dir + "/kvrocks.pid";
   if (log_dir.empty()) log_dir = dir;
   std::vector<std::string> create_dirs = {dir};
   for (const auto &name : create_dirs) {

src/config/config.h

@@ -122,6 +122,8 @@ struct Config {
   std::vector<std::string> binds;
   std::string dir;
   std::string db_dir;
+  std::string backup_dir;  // GUARD_BY(backup_mu_)
+  std::string pidfile;
   std::string backup_sync_dir;
   std::string checkpoint_dir;
   std::string sync_checkpoint_dir;
@@ -237,13 +239,9 @@ struct Config {
   void ClearMaster();
   bool IsSlave() const { return !master_host.empty(); }
   bool HasConfigFile() const { return !path_.empty(); }
-  std::string GetBackupDir() const { return backup_dir_.empty() ? dir + "/backup" : backup_dir_; }
-  std::string GetPidFile() const { return pidfile_.empty() ? dir + "/kvrocks.pid" : pidfile_; }
 
  private:
   std::string path_;
-  std::string backup_dir_;  // GUARD_BY(backup_mu_)
-  std::string pidfile_;
   std::string binds_str_;
   std::string slaveof_;
   std::string compact_cron_str_;

src/storage/storage.cc

@@ -384,7 +384,7 @@ Status Storage::Open(DBOpenMode mode) {
 Status Storage::CreateBackup(uint64_t *sequence_number) {
   LOG(INFO) << "[storage] Start to create new backup";
   std::lock_guard<std::mutex> lg(config_->backup_mu);
-  std::string task_backup_dir = config_->GetBackupDir();
+  std::string task_backup_dir = config_->backup_dir;
 
   std::string tmpdir = task_backup_dir + ".tmp";
   // Maybe there is a dirty tmp checkpoint, try to clean it
@@ -535,7 +535,7 @@ void Storage::EmptyDB() {
 void Storage::PurgeOldBackups(uint32_t num_backups_to_keep, uint32_t backup_max_keep_hours) {
   time_t now = util::GetTimeStamp();
   std::lock_guard<std::mutex> lg(config_->backup_mu);
-  std::string task_backup_dir = config_->GetBackupDir();
+  std::string task_backup_dir = config_->backup_dir;
 
   // Return if there is no backup
   auto s = env_->FileExists(task_backup_dir);

tests/cppunit/config_test.cc

@@ -175,6 +175,8 @@ TEST(Config, Rewrite) {
   redis::CommandTable::Reset();
   Config config;
   ASSERT_TRUE(config.Load(CLIOptions(path)).IsOK());
+  ASSERT_EQ(config.dir + "/backup", config.backup_dir);
+  ASSERT_EQ(config.dir + "/kvrocks.pid", config.pidfile);
   ASSERT_TRUE(config.Rewrite({}).IsOK());
   // Need to re-populate the command table since it has renamed by the previous
   redis::CommandTable::Reset();