fix: polling based execution config watcher #1671
Conversation
Router image scan passed✅ No security vulnerabilities found in image: |
endigma
force-pushed
the
jesse/eng-6304-switch-to-polling-based-file-watcher
branch
from
b74080a to
ec855ae
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the configuration file watcher to use a polling-based approach instead of relying on filesystem notifications. Key changes include the introduction of a new simple watcher, corresponding test improvements, and updates to configuration and router startup to support polling parameters.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| router/pkg/watcher/simple_watcher.go | New polling-based file watcher implementation |
| router/pkg/watcher/simple_watcher_test.go | Comprehensive tests for the new watcher functionality |
| router-tests/router_config_watch_test.go | Integration test updating the router config and monitoring hot-reload behavior |
| router/core/router.go | Switch from legacy file watcher to the new polling-based implementation, update error handling flow |
| router/pkg/config/config.go | Add WatchInterval support to config schema |
| router/cmd/instance.go | Pass WatchInterval configuration to the router execution config |
Comments suppressed due to low confidence (1)
router/core/router.go:1139
- The error message indicates a failure to 'serialize' the config file when the operation is actually unmarshaling. Consider updating the message to 'Failed to parse config file' or 'Failed to unmarshal config file' for clarity.
r.logger.Error("Failed to serialize config file", zap.Error(err))
5e876e2 to
bdbe017
Compare
endigma
force-pushed
the
jesse/eng-6304-switch-to-polling-based-file-watcher
branch
from
b9a1020 to
239915d
Compare
| Callback func() | ||
| } | ||
|
|
||
| func LogSimpleWatch(ctx context.Context, options SimpleWatcherOptions) { |
There was a problem hiding this comment.
as we basically require all options to be set, would it make sense to make them private and have a function NewSimpleWatcherOptions() with a bit of validation that returns an error instead of a panicking when calling Logger or Callback?
| @@ -1116,45 +1117,36 @@ func (r *Router) Start(ctx context.Context) error { | |||
| }() | |||
|
|
|||
| if r.executionConfig != nil && r.executionConfig.Watch { | |||
| go watcher.LogSimpleWatch(ctx, watcher.SimpleWatcherOptions{ | |||
There was a problem hiding this comment.
The old watcher is unused now, correct? Any reason to keep unused code?
| // Wait for the first cycle to complete to set baseline | ||
| time.Sleep(2 * watchInterval) | ||
|
|
||
| wg.Add(1) |
There was a problem hiding this comment.
Just from a review perspective wg here looks a bit like a race condition. I personally would try to avoid time.Sleep. I suggest adding something to the waitgroup first and waiting 2 times or you could also work with channels and a select with something like <-time.After.
What do you think?
Motivation and Context
inotifyandfsnotifyare great, until various not-quite-standard filesystems and symlinks get thrown into the mix, then it doesn't seem so great anymore.Theoretical limitations