feat(watcher): add support for poll watcher #21258
Conversation
|
Ameer Ibrahim seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
src/config/watcher.rs
Outdated
| @@ -19,21 +19,60 @@ const CONFIG_WATCH_DELAY: std::time::Duration = std::time::Duration::from_secs(1 | |||
|
|
|||
| const RETRY_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); | |||
|
|
|||
| pub enum ConfigWatcher { | |||
There was a problem hiding this comment.
I found confusing that this PR includes two ConfigWatcher struct with the same name but different meanings
There was a problem hiding this comment.
yeah , one i used enum for clap options and another one in watcher.
if u can suggest me a different name. i will change
src/config/watcher.rs
Outdated
| // Ok((watcher, receiver)) | ||
| // } | ||
|
|
||
| fn add_paths(watcher: &mut Box<dyn Watcher>, config_paths: &[PathBuf]) -> Result<(), Error> { |
There was a problem hiding this comment.
I think you could use
| fn add_paths(watcher: &mut Box<dyn Watcher>, config_paths: &[PathBuf]) -> Result<(), Error> { | |
| fn add_paths<T: Watcher>(watcher: &mut T, config_paths: &[PathBuf]) -> Result<(), Error> { |
Or it does not work with the boxed dyn Watcher?
|
Just as a recommendation, if your PR description contains the text |
src/cli.rs
Outdated
|
|
||
| pub fn watcher_config(method: ConfigWatcher, interval: NonZeroU64) -> watcher::ConfigWatcher { | ||
| match method { | ||
| ConfigWatcher::Inotify => watcher::ConfigWatcher::RecommendedWatcher, |
There was a problem hiding this comment.
the ConfigWatcher::Inotify variant does not always set the inotify watcher, it sets the RecommendedWatcher, which would be inotify on supported platforms and polling in others.
I think we should rename those variants (or at least, the Inotify one), as it is a bit misleading for the user (also, the default value in the args should be change accordingly)
amribm
force-pushed
the
poll_watcher_for_config_watch
branch
3 times, most recently
from
53b02e2
to
fb6766e
Compare
github-actions
bot
added
domain: topology
amribm
force-pushed
the
poll_watcher_for_config_watch
branch
from
fb6766e
to
2e702b3
Compare
|
Did you mean to close this @amribm ? |
|
i had some commits without my github email and CLA is not passing. in effort to ammend the username in commit . i messed up the tree. when i force pushed this PR got closed. Though i already created new one. #21290 |
Change for adding poll watcher for vector config change
Closes #21111