Allow manual specification of filewatcher behavior - #29
Conversation
Sorry, something went wrong.
dominikschubert
left a comment
There was a problem hiding this comment.
LGTM with some minor nits. Let's give users a few options to fine-tune things :)
Sorry, something went wrong.
| func NewChangeListener(debouncingInterval time.Duration) (*ChangeListener, error) { | ||
| watcher, err := filenotify.New(200 * time.Millisecond) | ||
| func NewChangeListener(debouncingInterval time.Duration, fileWatcher string) (*ChangeListener, error) { | ||
| watcher, err := filenotify.New(200*time.Millisecond, fileWatcher) |
There was a problem hiding this comment.
Should we maybe make the interval configurable as well while we're at it?
Sorry, something went wrong.
There was a problem hiding this comment.
I am kind of conflicted. On the one hand, it might make sense to do, if they want to increase it when running huge folders. On the other hand, I am not sure how much it will really impact.
Sorry, something went wrong.
joe4dev
left a comment
There was a problem hiding this comment.
LGTM 👍
Tested with tests.aws.services.lambda_.test_lambda_developer_tools.TestHotReloading.test_hot_reloading on macOS:
- Both polling and event work (e.g., LAMBDA_DOCKER_FLAGS=-e LOCALSTACK_FILE_WATCHER_STRATEGY=event)
- A wrong configuration does not crash the binary but just hangs 😒
- I needed to bump the sleeps from 0.6 to 1 second to make the test pass on macOS
Sorry, something went wrong.
| } else if fileWatcherStrategy == "polling" { | ||
| return NewPollingWatcher(interval), nil | ||
| } else { | ||
| log.Fatalf("Invalid filewatcher strategy %s. Only event and polling are allowed.\n", fileWatcherStrategy) |
There was a problem hiding this comment.
Would it make sense to crash the init binary rather than just log here?
Currently, it just hangs until a timeout kills the container :(
Sorry, something went wrong.
There was a problem hiding this comment.
A fatal log should indeed crash the init binary
Sorry, something went wrong.
There was a problem hiding this comment.
I see.
Then it's likely one of the error cases we do not (yet) handle actively in the init. Reporting back to LocalStack would be nice in the future to short-circuit the timeout (which is likely high when debugging).
Sorry, something went wrong.
Motivation
As seen with PR #28, there are some performance problems with the polling filewatcher.
It seems, that in newer macos versions, we have fsnotify events available as well. However, to avoid a braking change, and until we can verify what docker desktop version and which settings enable this, we are for now introducing an environment variable so users can manually override the automatic decision.
This will also be useful once we change the default behavior (e.g. with #28), to give users an option to go back to the current behavior.
Changes