Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(config): avoid rewriting the config file if it's unnecessary #2347

Merged
merged 2 commits into from
Jun 1, 2024
Merged

fix(config): avoid rewriting the config file if it's unnecessary #2347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4404a2
fix(config): avoid rewriting the config file if it's unnessary
git-hulk Jun 1, 2024
7c584a1
Add the read/write lock for the namespace
git-hulk Jun 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/commands/cmd_server.cc
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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);
git-hulk marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Loading