config: rewrite and persist config file #2153
Conversation
Signed-off-by: Ryan Leung <rleungx@gmail.com>
| return nil | ||
| } | ||
| var buf bytes.Buffer | ||
| if err := toml.NewEncoder(&buf).Encode(*new); err != nil { |
There was a problem hiding this comment.
Looks like it also writes out hidden configurations.
| slice := make([]interface{}, 0) | ||
| for _, str := range strSlice { | ||
| slice = append(slice, str) | ||
| // TODO: handle slice better |
There was a problem hiding this comment.
can you explain how it is not good enough now?
| // TODO: handle slice better | ||
| if item, ok := updateItem.(string); ok { | ||
| strSlice := strings.Split(item, ",") | ||
| slice := make([]interface{}, 0) |
There was a problem hiding this comment.
var slice []interface{} or make([]interface{}, 0, len(strSlice))
Signed-off-by: Ryan Leung <rleungx@gmail.com>
server/config/config.go
Outdated
| return err | ||
| } | ||
| // 0644 represent the permission of file which is "-rw-r--r--" here. | ||
| return ioutil.WriteFile(filePath, buf.Bytes(), 0644) |
There was a problem hiding this comment.
To avoid corrupted content during lost power, we should:
- write the content to temp file
- sync the temp file
- rename the temp file to the config file.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
| dir := filepath.Dir(filePath) | ||
| tmpfile := filepath.Join(dir, "tmp_pd.toml") | ||
|
|
||
| f, err := os.Create(tmpfile) |
There was a problem hiding this comment.
The default permission is 0666, should we change it to 0644?
There was a problem hiding this comment.
I think it's unnecessary?
Codecov Report
@@ Coverage Diff @@
## master #2153 +/- ##
==========================================
- Coverage 76.03% 75.99% -0.04%
==========================================
Files 195 195
Lines 20376 20405 +29
==========================================
+ Hits 15493 15507 +14
- Misses 3695 3710 +15
Partials 1188 1188
Continue to review full report at Codecov.
|
|
/merge |
sre-bot
added
the
status/can-merge
|
/run-all-tests |
|
@rleungx merge failed. |
|
/run-all-tests |
1 similar comment
|
/run-all-tests |
|
/merge |
|
/run-all-tests |
|
@rleungx merge failed. |
|
/merge |
|
/run-all-tests |
|
will the comments in the old config file get persisted in the updated config file? |
|
This feature has been removed, it won't rewrite the config file now |
Oh😱... |
|
AFAIK, If you use BurntSushi/toml, there are issues that have some solution to do it, but they are not the official way, otherwise, you need to implement it by yourself. |
got it, thx. |
What problem does this PR solve?
After we enable the dynamic config, we will rewrite the config file and persist it once the config has been updated. Related to tikv/tikv#6684
What is changed and how it works?
The PR adds a new function named
RewriteFileto do it.Check List
Tests