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

config rewrite bug #1396

Closed
1 of 2 tasks
iushas opened this issue Apr 19, 2023 · 6 comments · Fixed by #1397
Closed
1 of 2 tasks

config rewrite bug #1396

iushas opened this issue Apr 19, 2023 · 6 comments · Fixed by #1397
Labels
bug type bug good first issue Good for newcomers

Comments

@iushas
Copy link

iushas commented Apr 19, 2023

Search before asking

  • I had searched in the issues and found no similar issues.

Version

2.3.0

Minimal reproduce step

namespace add xxx
config rewrite

What did you expect to see?

no extra blank lines in kvrocks.conf

What did you see instead?

First create 1w namespace, then config rewrite.
There are 1w extra blank lines after config rewrite

Anything Else?

No response

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@iushas iushas added the bug type bug label Apr 19, 2023
@iushas
Copy link
Author

iushas commented Apr 19, 2023

image

@PragmaTwice PragmaTwice added the good first issue Good for newcomers label Apr 19, 2023
@enjoy-binbin
Copy link
Member

It seems that in addition to this problem, every time we perform a config rewrite, an additional newline is appended

@enjoy-binbin
Copy link
Member

enjoy-binbin commented Apr 20, 2023

so it look like file.eof() problem. see https://stackoverflow.com/questions/21647/reading-from-text-file-until-eof-repeats-last-line

change it to this seems work

diff --git a/src/config/config.cc b/src/config/config.cc
index 3e3c286..77825ae 100644
--- a/src/config/config.cc
+++ b/src/config/config.cc
@@ -829,8 +829,8 @@ Status Config::Rewrite() {
   std::ifstream file(path_);
   if (file.is_open()) {
     std::string raw_line;
-    while (!file.eof()) {
-      std::getline(file, raw_line);
+    while (std::getline(file, raw_line)) {
+      if (file.eof()) break;
       auto parsed = ParseConfigLine(raw_line);
       if (!parsed || parsed->first.empty()) {
         lines.emplace_back(raw_line);

@git-hulk
Copy link
Member

It seems that in addition to this problem, every time we perform a config rewrite, an additional newline is appended

yes

@enjoy-binbin
Copy link
Member

I tested the changes above locally, it will work (and it also can fix this issue)
should we need to change all while (!file.eof()), I see we still have a few places where we use this

@git-hulk
Copy link
Member

Yes, I think so. cc @PragmaTwice

enjoy-binbin added a commit to enjoy-binbin/kvrocks that referenced this issue Apr 20, 2023
In the old code, everytime we perform a config rewrite, an
additional newline is appended, see apache#1396

Because iostream::eof will only return true after reading the
end of the stream. It does not indicate, that the next read will
be the end of the stream.

So everytime config rewrite is performed, the old loop will
append a newline in `lines`, and then we will append it to the
conf file.

In addition, the same modification has been made to other similar
places. This PR fixes apache#1396
git-hulk pushed a commit that referenced this issue Apr 20, 2023
In the old code, every time we perform a config rewrite, an
additional newline is appended, see #1396

Because iostream::eof will only return true after reading the
end of the stream. It does not indicate, that the next read will
be the end of the stream.

So every time config rewrite is performed, the old loop will
append a newline in `lines`, and then we will append it to the
conf file.

In addition, the same modification has been made to other similar
places. This PR fixes #1396
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug type bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants