This is an automated email from the ASF dual-hosted git repository.
hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new ffc2ece7 Don't rewrite unchanged items into the configuration file
(#1882)
ffc2ece7 is described below
commit ffc2ece731e7243c3a54db4cff8bc3bffdbdce22
Author: hulk <[email protected]>
AuthorDate: Tue Nov 7 21:46:16 2023 +0800
Don't rewrite unchanged items into the configuration file (#1882)
Currently, config rewrite will also add some default configurations
like backup-dir, pidfile into the configuration file. Some of them
are for compatible purposes and don't expect to be seen by users.
So we resolve this by ignoring the configuration items
if they do not exist in the configuration file and the value was not
modified.
---
kvrocks.conf | 3 +-
src/cli/main.cc | 4 +--
src/config/config.cc | 25 ++++++++--------
src/config/config.h | 7 +++--
src/config/config_type.h | 68 +++++++++++++++++++++++++++++--------------
src/storage/storage.cc | 4 +--
tests/cppunit/compact_test.cc | 1 -
tests/cppunit/test_base.h | 1 -
8 files changed, 68 insertions(+), 45 deletions(-)
diff --git a/kvrocks.conf b/kvrocks.conf
index 7667511b..fa36d96a 100644
--- a/kvrocks.conf
+++ b/kvrocks.conf
@@ -68,7 +68,7 @@ repl-namespace-enabled no
# and dump the cluster nodes into the file if it was changed.
#
# Default: yes
-# persist-cluster-nodes-enabled yes
+persist-cluster-nodes-enabled yes
# Set the max number of connected clients at the same time. By default
# this limit is set to 10000 clients. However, if the server is not
@@ -135,7 +135,6 @@ log-retention-days -1
# When running in daemonize mode, kvrocks writes a PID file in
${CONFIG_DIR}/kvrocks.pid by
# default. You can specify a custom pid file location here.
# pidfile /var/run/kvrocks.pid
-pidfile ""
# You can configure a slave instance to accept writes or not. Writing against
# a slave instance may be useful to store some ephemeral data (because data
diff --git a/src/cli/main.cc b/src/cli/main.cc
index df0bf033..c03923e7 100644
--- a/src/cli/main.cc
+++ b/src/cli/main.cc
@@ -153,12 +153,12 @@ int main(int argc, char *argv[]) {
}
bool is_supervised = IsSupervisedMode(config.supervised_mode);
if (config.daemonize && !is_supervised) Daemonize();
- s = CreatePidFile(config.pidfile);
+ s = CreatePidFile(config.GetPidFile());
if (!s.IsOK()) {
LOG(ERROR) << "Failed to create pidfile: " << s.Msg();
return 1;
}
- auto pidfile_exit = MakeScopeExit([&config] { RemovePidFile(config.pidfile);
});
+ auto pidfile_exit = MakeScopeExit([&config] {
RemovePidFile(config.GetPidFile()); });
#ifdef ENABLE_OPENSSL
// initialize OpenSSL
diff --git a/src/config/config.cc b/src/config/config.cc
index 1c782193..f9cf8cf6 100644
--- a/src/config/config.cc
+++ b/src/config/config.cc
@@ -130,10 +130,10 @@ Config::Config() {
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, "")},
+ {"backup-dir", false, new StringField(&backup_dir_, "")},
{"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_, "")},
{"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)},
@@ -378,12 +378,6 @@ void Config::initFieldCallback() {
{"dir",
[this](Server *srv, const std::string &k, const std::string &v) ->
Status {
db_dir = dir + "/db";
- {
- 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";
@@ -397,8 +391,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.
@@ -751,10 +745,10 @@ 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 (backup_dir.empty()) backup_dir = dir + "/backup";
+ if (pidfile_.empty()) pidfile_ = dir + "/kvrocks.pid";
if (log_dir.empty()) log_dir = dir;
- if (pidfile.empty()) pidfile = dir + "/kvrocks.pid";
std::vector<std::string> create_dirs = {dir};
for (const auto &name : create_dirs) {
auto s = rocksdb::Env::Default()->CreateDirIfMissing(name);
@@ -859,6 +853,11 @@ Status Config::Set(Server *srv, std::string key, const
std::string &value) {
return Status::OK();
}
+bool Config::checkFieldValueIsDefault(const std::string &key, const
std::string &value) const {
+ auto iter = fields_.find(key);
+ return iter != fields_.end() && iter->second->Default() == value;
+}
+
Status Config::Rewrite(const std::map<std::string, std::string> &tokens) {
if (!HasConfigFile()) {
return {Status::NotOK, "the server is running without a config file"};
@@ -912,7 +911,7 @@ Status Config::Rewrite(const std::map<std::string,
std::string> &tokens) {
fmt::format_to(std::back_inserter(out_buf), "{}\n", line);
}
for (const auto &remain : new_config) {
- if (remain.second.empty()) continue;
+ if (remain.second.empty() || checkFieldValueIsDefault(remain.first,
remain.second)) continue;
fmt::format_to(std::back_inserter(out_buf), "{}\n",
DumpConfigLine({remain.first, remain.second}));
}
std::string tmp_path = path_ + ".tmp";
diff --git a/src/config/config.h b/src/config/config.h
index aa1f9f89..d3dc1442 100644
--- a/src/config/config.h
+++ b/src/config/config.h
@@ -119,12 +119,10 @@ 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_sync_dir;
std::string checkpoint_dir;
std::string sync_checkpoint_dir;
std::string log_dir;
- std::string pidfile;
std::string db_name;
std::string masterauth;
std::string requirepass;
@@ -228,9 +226,13 @@ 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_;
@@ -244,5 +246,6 @@ struct Config {
void initFieldCallback();
Status parseConfigFromPair(const std::pair<std::string, std::string> &input,
int line_number);
Status parseConfigFromString(const std::string &input, int line_number);
+ bool checkFieldValueIsDefault(const std::string &key, const std::string
&value) const;
Status finish();
};
diff --git a/src/config/config_type.h b/src/config/config_type.h
index 0b980823..e2918392 100644
--- a/src/config/config_type.h
+++ b/src/config/config_type.h
@@ -59,6 +59,7 @@ class ConfigField {
public:
ConfigField() = default;
virtual ~ConfigField() = default;
+ virtual std::string Default() const = 0;
virtual std::string ToString() const = 0;
virtual Status Set(const std::string &v) = 0;
virtual Status ToNumber(int64_t *n) const { return {Status::NotOK, "not
supported"}; }
@@ -77,8 +78,9 @@ class ConfigField {
class StringField : public ConfigField {
public:
- StringField(std::string *receiver, std::string s) : receiver_(receiver) {
*receiver_ = std::move(s); }
+ StringField(std::string *receiver, std::string s) : receiver_(receiver),
default_(s) { *receiver_ = std::move(s); }
~StringField() override = default;
+ std::string Default() const override { return default_; }
std::string ToString() const override { return *receiver_; }
Status Set(const std::string &v) override {
*receiver_ = v;
@@ -87,22 +89,20 @@ class StringField : public ConfigField {
private:
std::string *receiver_;
+ std::string default_;
};
class MultiStringField : public ConfigField {
public:
- MultiStringField(std::vector<std::string> *receiver,
std::vector<std::string> input) : receiver_(receiver) {
+ MultiStringField(std::vector<std::string> *receiver,
std::vector<std::string> input)
+ : receiver_(receiver), default_(input) {
*receiver_ = std::move(input);
this->config_type = ConfigType::MultiConfig;
}
~MultiStringField() override = default;
- std::string ToString() const override {
- std::string tmp;
- for (auto &p : *receiver_) {
- tmp += p + "\n";
- }
- return tmp;
- }
+
+ std::string Default() const override { return format(default_); }
+ std::string ToString() const override { return format(*receiver_); }
Status Set(const std::string &v) override {
receiver_->emplace_back(v);
return Status::OK();
@@ -110,16 +110,26 @@ class MultiStringField : public ConfigField {
private:
std::vector<std::string> *receiver_;
+ std::vector<std::string> default_;
+
+ static std::string format(const std::vector<std::string> &v) {
+ std::string tmp;
+ for (auto &p : v) {
+ tmp += p + "\n";
+ }
+ return tmp;
+ }
};
template <typename IntegerType>
class IntegerField : public ConfigField {
public:
IntegerField(IntegerType *receiver, IntegerType n, IntegerType min,
IntegerType max)
- : receiver_(receiver), min_(min), max_(max) {
+ : receiver_(receiver), default_(n), min_(min), max_(max) {
*receiver_ = n;
}
~IntegerField() override = default;
+ std::string Default() const override { return std::to_string(default_); }
std::string ToString() const override { return std::to_string(*receiver_); }
Status ToNumber(int64_t *n) const override {
*n = *receiver_;
@@ -134,14 +144,19 @@ class IntegerField : public ConfigField {
private:
IntegerType *receiver_;
+ IntegerType default_ = 0;
IntegerType min_ = std::numeric_limits<IntegerType>::min();
IntegerType max_ = std::numeric_limits<IntegerType>::max();
};
class OctalField : public ConfigField {
public:
- OctalField(int *receiver, int n, int min, int max) : receiver_(receiver),
min_(min), max_(max) { *receiver_ = n; }
+ OctalField(int *receiver, int n, int min, int max) : receiver_(receiver),
default_(n), min_(min), max_(max) {
+ *receiver_ = n;
+ }
~OctalField() override = default;
+
+ std::string Default() const override { return fmt::format("{:o}", default_);
}
std::string ToString() const override { return fmt::format("{:o}",
*receiver_); }
Status ToNumber(int64_t *n) const override {
*n = *receiver_;
@@ -156,14 +171,17 @@ class OctalField : public ConfigField {
private:
int *receiver_;
+ int default_ = 0;
int min_ = INT_MIN;
int max_ = INT_MAX;
};
class YesNoField : public ConfigField {
public:
- YesNoField(bool *receiver, bool b) : receiver_(receiver) { *receiver_ = b; }
+ YesNoField(bool *receiver, bool b) : receiver_(receiver), default_(b) {
*receiver_ = b; }
~YesNoField() override = default;
+
+ std::string Default() const override { return default_ ? "yes" : "no"; }
std::string ToString() const override { return *receiver_ ? "yes" : "no"; }
Status ToBool(bool *b) const override {
*b = *receiver_;
@@ -182,6 +200,7 @@ class YesNoField : public ConfigField {
private:
bool *receiver_;
+ bool default_;
};
template <typename Enum>
@@ -190,16 +209,13 @@ class EnumField : public ConfigField {
using EnumItem = ConfigEnum<Enum>;
using EnumItems = std::vector<EnumItem>;
- EnumField(Enum *receiver, EnumItems enums, Enum e) : receiver_(receiver),
enums_(std::move(enums)) { *receiver_ = e; }
- ~EnumField() override = default;
-
- std::string ToString() const override {
- for (const auto &e : enums_) {
- if (e.val == *receiver_) return e.name;
- }
- return {};
+ EnumField(Enum *receiver, EnumItems enums, Enum e) : receiver_(receiver),
default_(e), enums_(std::move(enums)) {
+ *receiver_ = e;
}
+ ~EnumField() override = default;
+ std::string Default() const override { return enumToString(default_); }
+ std::string ToString() const override { return enumToString(*receiver_); }
Status ToNumber(int64_t *n) const override {
*n = static_cast<int64_t>(*receiver_);
return Status::OK();
@@ -213,16 +229,24 @@ class EnumField : public ConfigField {
}
}
- auto acceptale_values =
+ auto acceptable_values =
std::accumulate(enums_.begin(), enums_.end(), std::string{},
[this](const std::string &res, const EnumItem &e) {
if (&e != &enums_.back()) return res + "'" + e.name + "', ";
return res + "'" + e.name + "'";
});
- return {Status::NotOK, fmt::format("invalid enum option, acceptable values
are {}", acceptale_values)};
+ return {Status::NotOK, fmt::format("invalid enum option, acceptable values
are {}", acceptable_values)};
}
private:
Enum *receiver_;
+ Enum default_;
EnumItems enums_;
+
+ std::string enumToString(const Enum v) const {
+ for (const auto &e : enums_) {
+ if (e.val == v) return e.name;
+ }
+ return {};
+ }
};
diff --git a/src/storage/storage.cc b/src/storage/storage.cc
index 5710823c..eb0be132 100644
--- a/src/storage/storage.cc
+++ b/src/storage/storage.cc
@@ -336,7 +336,7 @@ Status Storage::Open(bool read_only) {
Status Storage::CreateBackup() {
LOG(INFO) << "[storage] Start to create new backup";
std::lock_guard<std::mutex> lg(config_->backup_mu);
- std::string task_backup_dir = config_->backup_dir;
+ std::string task_backup_dir = config_->GetBackupDir();
std::string tmpdir = task_backup_dir + ".tmp";
// Maybe there is a dirty tmp checkpoint, try to clean it
@@ -486,7 +486,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_->backup_dir;
+ std::string task_backup_dir = config_->GetBackupDir();
// Return if there is no backup
auto s = env_->FileExists(task_backup_dir);
diff --git a/tests/cppunit/compact_test.cc b/tests/cppunit/compact_test.cc
index c482173f..c5616304 100644
--- a/tests/cppunit/compact_test.cc
+++ b/tests/cppunit/compact_test.cc
@@ -30,7 +30,6 @@
TEST(Compact, Filter) {
Config config;
config.db_dir = "compactdb";
- config.backup_dir = "compactdb/backup";
config.slot_id_encoded = false;
auto storage = std::make_unique<engine::Storage>(&config);
diff --git a/tests/cppunit/test_base.h b/tests/cppunit/test_base.h
index 5d7dbe76..5c46f317 100644
--- a/tests/cppunit/test_base.h
+++ b/tests/cppunit/test_base.h
@@ -38,7 +38,6 @@ class TestBase : public testing::Test { // NOLINT
auto s = config_->Load(CLIOptions(path));
config_->db_dir = "testdb";
- config_->backup_dir = "testdb/backup";
config_->rocks_db.compression = rocksdb::CompressionType::kNoCompression;
config_->rocks_db.write_buffer_size = 1;
config_->rocks_db.block_size = 100;
