Skip to content

Commit

Permalink
fix(config): avoid rewriting the config file if it's unnecessary (#2347)
Browse files Browse the repository at this point in the history 
The server will start by rewriting the config file to persist namespace/token pairs
if namespace replication is enabled. But it's unnecessary if the namespace replication
is disabled or no tokens are loaded from the configuration file because the purpose of
this rewrite is to remove tokens from the config file and write them to the database.
  • Loading branch information
@git-hulk
git-hulk committed Jun 1, 2024
1 parent d2e0feb commit c65e505
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 41 deletions.
2 changes: 1 addition & 1 deletion src/commands/cmd_server.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1275,7 +1275,7 @@ REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandAuth>("auth", 2, "read-only ok-loadin
MakeCmdAttr<CommandInfo>("info", -1, "read-only ok-loading", 0, 0, 0),
MakeCmdAttr<CommandRole>("role", 1, "read-only ok-loading", 0, 0, 0),
MakeCmdAttr<CommandConfig>("config", -2, "read-only", 0, 0, 0, GenerateConfigFlag),
MakeCmdAttr<CommandNamespace>("namespace", -3, "read-only exclusive", 0, 0, 0),
MakeCmdAttr<CommandNamespace>("namespace", -3, "read-only", 0, 0, 0),
MakeCmdAttr<CommandKeys>("keys", 2, "read-only", 0, 0, 0),
MakeCmdAttr<CommandFlushDB>("flushdb", 1, "write no-dbsize-check", 0, 0, 0),
MakeCmdAttr<CommandFlushAll>("flushall", 1, "write no-dbsize-check", 0, 0, 0),
Expand Down
11 changes: 10 additions & 1 deletion src/config/config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -885,11 +885,20 @@ Status Config::Set(Server *srv, std::string key, const std::string &value) {
if (!s.IsOK()) return s.Prefixed("invalid value");
}

auto origin_value = field->ToString();
auto s = field->Set(value);
if (!s.IsOK()) return s.Prefixed("failed to set new value");

if (field->callback) {
return field->callback(srv, key, value);
s = field->callback(srv, key, value);
if (!s.IsOK()) {
// rollback the value if the callback failed
auto set_status = field->Set(origin_value);
if (!set_status.IsOK()) {
return set_status.Prefixed("failed to rollback the value");
}
}
return s;
}

return Status::OK();
Expand Down
93 changes: 57 additions & 36 deletions src/server/namespace.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,40 +51,53 @@ bool Namespace::IsAllowModify() const {
return config->HasConfigFile() || config->repl_namespace_enabled;
}

Status Namespace::loadFromDB(std::map<std::string, std::string>* db_tokens) const {
std::string value;
auto s = storage_->Get(rocksdb::ReadOptions(), cf_, kNamespaceDBKey, &value);
if (!s.ok()) {
if (s.IsNotFound()) return Status::OK();
return {Status::NotOK, s.ToString()};
}

jsoncons::json j = jsoncons::json::parse(value);
for (const auto& iter : j.object_range()) {
db_tokens->insert({iter.key(), iter.value().as_string()});
}
return Status::OK();
}

Status Namespace::LoadAndRewrite() {
auto config = storage_->GetConfig();
// Namespace is NOT allowed in the cluster mode, so we don't need to rewrite here.
if (config->cluster_enabled) return Status::OK();

// Load from the configuration file first
tokens_ = config->load_tokens;
std::map<std::string, std::string> db_tokens;
auto s = loadFromDB(&db_tokens);
if (!s.IsOK()) return s;

// We would like to load namespaces from db even if repl_namespace_enabled is false,
// this can avoid missing some namespaces when turn on/off repl_namespace_enabled.
std::string value;
auto s = storage_->Get(rocksdb::ReadOptions(), cf_, kNamespaceDBKey, &value);
if (!s.ok() && !s.IsNotFound()) {
return {Status::NotOK, s.ToString()};
if (!db_tokens.empty() && !config->repl_namespace_enabled) {
return {Status::NotOK, "cannot switch off repl_namespace_enabled when namespaces exist in db"};
}
if (s.ok()) {
// The namespace db key is existed, so it doesn't allow to switch off repl_namespace_enabled
if (!config->repl_namespace_enabled) {
return {Status::NotOK, "cannot switch off repl_namespace_enabled when namespaces exist in db"};
}

jsoncons::json j = jsoncons::json::parse(value);
for (const auto& iter : j.object_range()) {
if (tokens_.find(iter.key()) == tokens_.end()) {
// merge the namespace from db
tokens_[iter.key()] = iter.value().as<std::string>();
}
std::unique_lock<std::shared_mutex> lock(tokens_mu_);
// Load from the configuration file first
tokens_ = config->load_tokens;
// Merge the tokens from the database if the token is not in the configuration file
for (const auto& iter : db_tokens) {
if (tokens_.find(iter.first) == tokens_.end()) {
tokens_[iter.first] = iter.second;
}
}

return Rewrite();
// The following rewrite is to remove namespace/token pairs from the configuration if the namespace replication
// is enabled. So we don't need to do that if no tokens are loaded or the namespace replication is disabled.
if (config->load_tokens.empty() || !config->repl_namespace_enabled) return Status::OK();

return Rewrite(tokens_);
}

StatusOr<std::string> Namespace::Get(const std::string& ns) const {
StatusOr<std::string> Namespace::Get(const std::string& ns) {
std::shared_lock lock(tokens_mu_);
for (const auto& iter : tokens_) {
if (iter.second == ns) {
return iter.first;
Expand All @@ -93,7 +106,8 @@ StatusOr<std::string> Namespace::Get(const std::string& ns) const {
return {Status::NotFound};
}

StatusOr<std::string> Namespace::GetByToken(const std::string& token) const {
StatusOr<std::string> Namespace::GetByToken(const std::string& token) {
std::shared_lock lock(tokens_mu_);
auto iter = tokens_.find(token);
if (iter == tokens_.end()) {
return {Status::NotFound};
Expand Down Expand Up @@ -121,6 +135,7 @@ Status Namespace::Set(const std::string& ns, const std::string& token) {
return {Status::NotOK, kErrInvalidToken};
}

std::unique_lock lock(tokens_mu_);
for (const auto& iter : tokens_) {
if (iter.second == ns) { // need to delete the old token first
tokens_.erase(iter.first);
Expand All @@ -129,7 +144,7 @@ Status Namespace::Set(const std::string& ns, const std::string& token) {
}
tokens_[token] = ns;

s = Rewrite();
s = Rewrite(tokens_);
if (!s.IsOK()) {
tokens_.erase(token);
return s;
Expand All @@ -138,17 +153,22 @@ Status Namespace::Set(const std::string& ns, const std::string& token) {
}

Status Namespace::Add(const std::string& ns, const std::string& token) {
// duplicate namespace
for (const auto& iter : tokens_) {
if (iter.second == ns) {
if (iter.first == token) return Status::OK();
return {Status::NotOK, kErrNamespaceExists};
{
std::shared_lock lock(tokens_mu_);
// duplicate namespace
for (const auto& iter : tokens_) {
if (iter.second == ns) {
if (iter.first == token) return Status::OK();
return {Status::NotOK, kErrNamespaceExists};
}
}
// duplicate token
if (tokens_.find(token) != tokens_.end()) {
return {Status::NotOK, kErrTokenExists};
}
}
// duplicate token
if (tokens_.find(token) != tokens_.end()) {
return {Status::NotOK, kErrTokenExists};
}

// we don't need to lock the mutex here because the Set method will lock it
return Set(ns, token);
}

Expand All @@ -160,10 +180,11 @@ Status Namespace::Del(const std::string& ns) {
return {Status::NotOK, kErrCantModifyNamespace};
}

std::unique_lock lock(tokens_mu_);
for (const auto& iter : tokens_) {
if (iter.second == ns) {
tokens_.erase(iter.first);
auto s = Rewrite();
auto s = Rewrite(tokens_);
if (!s.IsOK()) {
tokens_[iter.first] = iter.second;
return s;
Expand All @@ -174,11 +195,11 @@ Status Namespace::Del(const std::string& ns) {
return {Status::NotOK, kErrNamespaceNotFound};
}

Status Namespace::Rewrite() {
Status Namespace::Rewrite(const std::map<std::string, std::string>& tokens) const {
auto config = storage_->GetConfig();
// Rewrite the configuration file only if it's running with the configuration file
if (config->HasConfigFile()) {
auto s = config->Rewrite(tokens_);
auto s = config->Rewrite(tokens);
if (!s.IsOK()) {
return s;
}
Expand All @@ -195,7 +216,7 @@ Status Namespace::Rewrite() {
return Status::OK();
}
jsoncons::json json;
for (const auto& iter : tokens_) {
for (const auto& iter : tokens) {
json[iter.first] = iter.second;
}
return storage_->WriteToPropagateCF(kNamespaceDBKey, json.to_string());
Expand Down
10 changes: 7 additions & 3 deletions src/server/namespace.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,21 @@ class Namespace {
Namespace &operator=(const Namespace &) = delete;

Status LoadAndRewrite();
StatusOr<std::string> Get(const std::string &ns) const;
StatusOr<std::string> GetByToken(const std::string &token) const;
StatusOr<std::string> Get(const std::string &ns);
StatusOr<std::string> GetByToken(const std::string &token);
Status Set(const std::string &ns, const std::string &token);
Status Add(const std::string &ns, const std::string &token);
Status Del(const std::string &ns);
const std::map<std::string, std::string> &List() const { return tokens_; }
Status Rewrite();
Status Rewrite(const std::map<std::string, std::string> &tokens) const;
bool IsAllowModify() const;

private:
engine::Storage *storage_;
rocksdb::ColumnFamilyHandle *cf_ = nullptr;

std::shared_mutex tokens_mu_;
std::map<std::string, std::string> tokens_;

Status loadFromDB(std::map<std::string, std::string> *db_tokens) const;
};
9 changes: 9 additions & 0 deletions tests/gocase/unit/namespace/namespace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,16 @@ func TestNamespaceReplicate(t *testing.T) {
})

t.Run("Turn off namespace replication is not allowed", func(t *testing.T) {
r := masterRdb.Do(ctx, "NAMESPACE", "ADD", "test-ns", "ns-token")
require.NoError(t, r.Err())
require.Equal(t, "OK", r.Val())
util.ErrorRegexp(t, masterRdb.ConfigSet(ctx, "repl-namespace-enabled", "no").Err(), ".*cannot switch off repl_namespace_enabled when namespaces exist in db.*")

// it should be allowed after deleting all namespaces
r = masterRdb.Do(ctx, "NAMESPACE", "DEL", "test-ns")
require.NoError(t, r.Err())
require.Equal(t, "OK", r.Val())
require.NoError(t, masterRdb.ConfigSet(ctx, "repl-namespace-enabled", "no").Err())
})
}

Expand Down

0 comments on commit c65e505

Please sign in to comment.