Allow manual specification of filewatcher behavior#29
Uh oh!
There was an error while loading. Please reload this page.
Conversation
dominikschubert left a comment
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 :)
Uh oh!
There was an error while loading. Please reload this page.
cmd/localstack/hotreloading.go Outdated
| funcNewChangeListener(debouncingInterval time.Duration) (*ChangeListener, error){ | ||
| watcher, err:=filenotify.New(200*time.Millisecond) | ||
| funcNewChangeListener(debouncingInterval time.Duration, fileWatcherstring) (*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?
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.
joe4dev left a comment
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
pollingandeventwork (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
| } elseiffileWatcherStrategy=="polling"{ | ||
| returnNewPollingWatcher(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 :(
There was a problem hiding this comment.
A fatal log should indeed crash the init binary
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).
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
LOCALSTACK_FILE_WATCHER_STRATEGYconfig, with to possible options:eventandpolling. Event uses file system events, polling our poller like currently on Docker Desktop. Specifying this option will override the default handling, if it is empty, the current behavior is preserved. If set to something other thaneventorpolling, the init binary will crash to indicate an incorrect configuration.