feat(watcher): add support for poll watcher #21290

amribm · 2024-09-13T04:59:49Z

bits-bot · 2024-09-13T04:59:54Z

All committers have signed the CLA.

src/app.rs

jorgehermo9 · 2024-09-13T06:52:18Z

src/app.rs

+    interval: NonZeroU64,
+) -> config::watcher::WatcherConfig {
+    match method {
+        WatchConfigMethod::Inotify => config::watcher::WatcherConfig::RecommendedWatcher,


Please, take a look into my comment about this in your closed PR

#21258 (comment)

Can we put this as Recommended or any thing that comes to ur mind? @jszwedko

yes, that would fit

src/config/watcher.rs

jorgehermo9 · 2024-09-13T13:50:09Z

src/config/watcher.rs

+        }
+        WatcherConfig::PollWatcher(interval) => {
+            let config =
+                notify::Config::default().with_poll_interval(Duration::from_secs(*interval));


Should we allow the user to specify compare_contents? https://docs.rs/notify/latest/notify/struct.Config.html#method.with_compare_contents
maybe another argument? Im afraid we are adding too much arguments for this though

@jszwedko

I'd vote against it being user controllable and hope for it to just have a sane default. I can't think of a scenario where I'd wanna toggle it!

We found issues in openshift environments where the inotify watcher was constantly triggered and the file contents did not change at all. In this case, we would like to hash the contents to check if it really changed before reloading vector config. I think having sane defaults is good (for example, this would default to false), but there are situations where this is useful

Would there be a downside to always hashing the changes?

Yes, as the docs I pointed states:

By enabling this feature, performance will be significantly impacted as all files will need to be read and hashed at each poll_interval.

I prefer that downside to be opt-in

jorgehermo9 · 2024-09-14T11:53:32Z

src/config/watcher.rs

+
+enum Watcher {
+    /// recommended watcher for os, usually inotify for linux based systems
+    RecommendedWatcher(notify::RecommendedWatcher),


I think it would be more idiomatic to use only RecommendedWatcher, adding use notify::RecommendedWatcher and the top of the file (as you has before)

Suggested change

RecommendedWatcher(notify::RecommendedWatcher),

RecommendedWatcher(RecommendedWatcher),

My previous suggestion about this was only about the notify::Watcher trait, and because it conflicted with your Watcher enum

Since the Watcher is necessary for use watch method on PollWatcher and RecommendWatcher . removing that makes watch method is not defined error

I think you can use notify::RecommendedWatcher and use notify::PollingWatcher, that have nothing related to what you're saying about the Watcher trait. The previous code imported those two structs

and indeed, maybe you need to use notify::Watcher to use the watch method

jorgehermo9 · 2024-09-14T11:53:42Z

src/config/watcher.rs

+    /// recommended watcher for os, usually inotify for linux based systems
+    RecommendedWatcher(notify::RecommendedWatcher),
+    /// poll based watcher. for watching files from NFS.
+    PollWatcher(notify::PollWatcher),


Suggested change

PollWatcher(notify::PollWatcher),

PollWatcher(PollWatcher),

src/config/watcher.rs

jorgehermo9 · 2024-09-14T12:02:55Z

src/config/watcher.rs

+
+    fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<(), Error> {
+        match self {
+            &mut Watcher::RecommendedWatcher(ref mut watcher) => {


I think you can get rid of this (In the other match arm too)

This behaviour was improved a few years ago

Read about it here: https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md

Suggested change

&mut Watcher::RecommendedWatcher(ref mut watcher) => {

Watcher::RecommendedWatcher(watcher) => {

jorgehermo9 · 2024-09-14T12:04:54Z

src/config/watcher.rs

+    match watcher_conf {
+        WatcherConfig::RecommendedWatcher => {
+            let recommended_watcher = recommended_watcher(sender)?;
+            let mut watcher = Watcher::RecommendedWatcher(recommended_watcher);
+            watcher.add_paths(config_paths)?;
+            Ok((watcher, receiver))
+        }
+        WatcherConfig::PollWatcher(interval) => {
+            let config =
+                notify::Config::default().with_poll_interval(Duration::from_secs(*interval));
+            let poll_watcher = notify::PollWatcher::new(sender, config)?;
+            let mut watcher = Watcher::PollWatcher(poll_watcher);
+            watcher.add_paths(config_paths)?;
+            Ok((watcher, receiver))
+        }


I think you can take

watcher.add_paths(config_paths)?; Ok((watcher, receiver))

out of the match branches

Suggested change

match watcher_conf {

WatcherConfig::RecommendedWatcher => {

let recommended_watcher = recommended_watcher(sender)?;

let mut watcher = Watcher::RecommendedWatcher(recommended_watcher);

watcher.add_paths(config_paths)?;

Ok((watcher, receiver))

}

WatcherConfig::PollWatcher(interval) => {

let config =

notify::Config::default().with_poll_interval(Duration::from_secs(*interval));

let poll_watcher = notify::PollWatcher::new(sender, config)?;

let mut watcher = Watcher::PollWatcher(poll_watcher);

watcher.add_paths(config_paths)?;

Ok((watcher, receiver))

}

let watcher = match watcher_conf {

WatcherConfig::RecommendedWatcher => {

let recommended_watcher = recommended_watcher(sender)?;

Watcher::RecommendedWatcher(recommended_watcher)

}

WatcherConfig::PollWatcher(interval) => {

let config =

notify::Config::default().with_poll_interval(Duration::from_secs(*interval));

let poll_watcher = notify::PollWatcher::new(sender, config)?;

Watcher::PollWatcher(poll_watcher)

};

watcher.add_paths(config_paths)?;

Ok((watcher, receiver))

yeah, this seems elegant. i'll do that

jorgehermo9 · 2024-09-14T12:09:34Z

src/cli.rs

+        env = "VECTOR_WATCH_CONFIG_POLL_INTERVAL_SECONDS",
+        default_value = "30"
+    )]
+    pub watch_config_poll_interval_seconds: NonZeroU64,


I think we should update this documentation to explain the polling watcher and when to use it

https://vector.dev/docs/administration/management/#automatic-reloading-on-configuration-change

But first lets see what maintainers think about documenting this

Co-authored-by: Jorge Hermo <[email protected]>

amribm requested a review from a team as a code owner September 13, 2024 04:59

feat(watcher): change for add poll watcher support

ae66312

amribm force-pushed the feature/support_for_poll_watcher branch from 3773b14 to ae66312 Compare September 13, 2024 05:01

fix(watcher): fix spelling

1802ece

amribm force-pushed the feature/support_for_poll_watcher branch from 4c926d0 to 1802ece Compare September 13, 2024 05:17

amribm changed the title ~~feat(watcher): add support for poll watcher support~~ feat(watcher): add support for poll watcher Sep 13, 2024

jorgehermo9 reviewed Sep 13, 2024

View reviewed changes

amribm mentioned this pull request Sep 13, 2024

feat(watcher): add support for poll watcher #21258

Closed

jorgehermo9 reviewed Sep 13, 2024

View reviewed changes

Add(watcher): change for PR

ed86d4a

jorgehermo9 reviewed Sep 14, 2024

View reviewed changes

Update src/config/watcher.rs

4bee121

Co-authored-by: Jorge Hermo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(watcher): add support for poll watcher #21290

feat(watcher): add support for poll watcher #21290

amribm commented Sep 13, 2024

bits-bot commented Sep 13, 2024 •

edited

Loading

jorgehermo9 Sep 13, 2024

amribm Sep 13, 2024 •

edited

Loading

jorgehermo9 Sep 13, 2024

jorgehermo9 Sep 13, 2024

brittonhayes Sep 13, 2024

jorgehermo9 Sep 14, 2024 •

edited

Loading

brittonhayes Sep 14, 2024

jorgehermo9 Sep 14, 2024 •

edited

Loading

jorgehermo9 Sep 14, 2024

amribm Sep 15, 2024

jorgehermo9 Sep 15, 2024

jorgehermo9 Sep 14, 2024

jorgehermo9 Sep 14, 2024

jorgehermo9 Sep 14, 2024

amribm Sep 15, 2024

jorgehermo9 Sep 14, 2024

	RecommendedWatcher(notify::RecommendedWatcher),
	RecommendedWatcher(RecommendedWatcher),

	&mut Watcher::RecommendedWatcher(ref mut watcher) => {
	Watcher::RecommendedWatcher(watcher) => {

feat(watcher): add support for poll watcher #21290

Are you sure you want to change the base?

feat(watcher): add support for poll watcher #21290

Conversation

amribm commented Sep 13, 2024

bits-bot commented Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

amribm Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgehermo9 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgehermo9 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bits-bot commented Sep 13, 2024 •

edited

Loading

amribm Sep 13, 2024 •

edited

Loading

jorgehermo9 Sep 14, 2024 •

edited

Loading

jorgehermo9 Sep 14, 2024 •

edited

Loading