Implement backup mechanism for config #288
Conversation
|
Questions:
|
| @@ -440,6 +466,7 @@ private void _UpdateConfigToV15() | |||
| private void _SaveCurrentConfig() | |||
| { | |||
| File.WriteAllText(ConfigPath, JsonConvert.SerializeObject(_config, Formatting.Indented)); | |||
| File.WriteAllText(ConfigBackupPath, JsonConvert.SerializeObject(_config, Formatting.Indented)); | |||
There was a problem hiding this comment.
I think backup should be saved in e separate function after updating is done. Because the case was config was corrupted during updating the config. So, in current implementation, backup could also be corrupted, right?
There was a problem hiding this comment.
Steps during config update:
- Copy old config to config backup
- Write new data to config
- Delete backup
I don't think we need separate function for this, but step order is important
There was a problem hiding this comment.
okay, I did it slightly different way. I'm always keeping a backup file. This way, both the Config file and the backup file cannot be corrupted at the same time. In case there is a problem updating the config file, the backup file will still be intact.
I think this solution will handle your case and also handle any case where the config file is somehow corrupted.
| catch (Exception e) { | ||
| try { | ||
| Logger.LogWarning($"Could not load config from {ConfigPath}. Trying from backup ({ConfigBackupPath})"); | ||
| var configLoader = new LocalConfigLoader(ConfigBackupPath); |
There was a problem hiding this comment.
In this case you need to copy backup file to the config file
There was a problem hiding this comment.
The config will be saved to the config file in the following call to _SaveCurrentConfig
There was a problem hiding this comment.
It is not guaranteed really (for example, exceptions on the intermediate steps). It is better to copy this file explicitly
There was a problem hiding this comment.
done in the latest commit.
|
One more thing. It was not in original task conditions, but save config in constructor only if it was really changed (add modify flag and set it if you change any data) |
I have now called save config only if something is changed. |
Anachor
force-pushed
the
config-backup
branch
from
57338a9 to
7e1f012
Compare
There was a problem hiding this comment.
Tests are failing due to the fail to find config.json.bak: https://app.travis-ci.com/github/LATOKEN/lachain/builds/256617012 Fix it please, this class should not require bak file existence
|
It seems that the tests are failing because of the new test I added. But it should not happen if tests are completely independent. In my test, I try deleting config file and backup to see if everything works correctly. This deletion is causing problem in other tests. |
|
I think we don't need such test in unit test really. It is enough to test it manually. Unit tests share the same setup, it is dangerous to delete common config |
|
Removed the offending lines in the test. All checks pass now. |
Anachor
force-pushed
the
config-backup
branch
from
e2db0a2 to
3d89a9c
Compare
ConfigPath. If failed, tried fromConfigBackupPath. If failed again, throw exception.ConfigPathandConfigBackupPathSaveConfigafter loading to sync config and backup