-
Notifications
You must be signed in to change notification settings - Fork 492
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
[refactoring] Introduce ConfigManager
1/n
#11875
[refactoring] Introduce ConfigManager
1/n
#11875
Conversation
@@ -38,26 +38,6 @@ public void AssertFilePathSet() | |||
} | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public bool CheckFileChange() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only used by Backend in ConfigWatcher
. Moved to ConfigManager
.
throw new FileNotFoundException($"{GetType().Name} file did not exist at path: `{FilePath}`."); | ||
} | ||
|
||
lock (FileLock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lock has been removed. The rationale is that Backend is supposed to
- Read its config file.
- Use
ConfigWatcher
to re-load its configuration.
No other code is supposed to touch the config file. For this reason, I believe the change is safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
Client recognizes if restart is needed after the config change.
Backend can load config changes on the fly.
public async Task CheckFileChangeTestAsync() | ||
{ | ||
string workDirectory = await Common.GetEmptyWorkDirAsync(); | ||
string configPath = Path.Join(workDirectory, "testConfig.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: How come you use Path.Join
and not Path.Combine
? Both works fine, I'm just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just forgot that we use Path.Combine
more often. However, the two methods behave the same here so there is no difference really AFAIK.
Follow-up to #11860