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.

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

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

Merged
merged 2 commits into from
Jun 1, 2024

Conversation

git-hulk
Copy link
Member

@git-hulk git-hulk commented Jun 1, 2024

The server start will rewrite the config file to persist namespace/token pairs
if the namespace replication is enabled. But it's unnecessary if the namespace replication
is disabled or no tokens were loaded from the configuration file because the purpose of
this rewrite is to remove tokens from the config file and write to the Database.

src/config/config.cc Outdated Show resolved Hide resolved
@git-hulk git-hulk force-pushed the avoid-unnessary-config-rewrite branch from 26ac817 to d6666f1 Compare June 1, 2024 08:41
The server start will rewrite the config file to presist namespace/token pairs
if the namespace replication is enabled. But it's unnecessary if the
namespace replication is diabled or no tokens were loaded from the
configuration file, because the purpose of this rewrite is to remove
tokens from the config file and write to the Database.
@git-hulk git-hulk force-pushed the avoid-unnessary-config-rewrite branch from d6666f1 to d4404a2 Compare June 1, 2024 09:01
@PragmaTwice
Copy link
Member

PragmaTwice commented Jun 1, 2024

It can fix the problem when there's no namespace in kvrocks except the default one.

But what if there are some namespaces? We also need to consider this situation.

@git-hulk
Copy link
Member Author

git-hulk commented Jun 1, 2024

But what if there are some namespaces? We also need to consider this situation.

Rewrite() will persist them if there are any namespaces(except the default), and then remove them from the config file. So it should be good for this scenario.

PragmaTwice
PragmaTwice previously approved these changes Jun 1, 2024
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

PragmaTwice
PragmaTwice previously approved these changes Jun 1, 2024
@git-hulk git-hulk force-pushed the avoid-unnessary-config-rewrite branch from c62ab5d to 7c584a1 Compare June 1, 2024 15:13
@git-hulk git-hulk requested a review from PragmaTwice June 1, 2024 16:11
@git-hulk git-hulk merged commit c65e505 into apache:unstable Jun 1, 2024
32 checks passed
Copy link

sonarcloud bot commented Jun 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants