From 7bea824c0d0a5f428174def4db89a3ce9869f0ce Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Tue, 14 May 2024 11:14:39 -0700 Subject: [PATCH] feat: replace offline-mode filewatcher with polling (#317) We've encountered robustness issues with the current offline-mode file-watcher implementation in Docker images that use shared volumes. Additionally, we're seeing intermittent CI failures in the file-watcher tests. Overall, I believe the complexity introduced by file-watcher isn't worth the payoff. It is supposed to unify all the different operating system methods of notifying that files have changed, but it falls short. The vast majority of the latency when using offline mode would be downloading the actual archive from LaunchDarkly and the interval between those downloads - which might be minutes/hours/days. This commit replaces the file-watcher with a simple polling loop. Every interval, it calls `stat()` and determines if the offline archive needs to be reloaded. The default interval is 1 second, and the minimum configurable is 100ms. The minimum was chosen to protect the system in case of accidental configuration of an extremely short interval (like 0). In practice, most users may raise the interval - for example, if they're fetching the archive every hour, there is no need to use a 1 second interval. --- config/config.go | 18 +- config/config_validation.go | 24 ++- config/test_data_configs_invalid_test.go | 15 ++ config/test_data_configs_valid_test.go | 33 +++- docs/configuration.md | 15 +- internal/filedata/archive_manager.go | 170 +++++------------- internal/filedata/archive_manager_test.go | 8 +- .../archive_manager_test_base_test.go | 4 +- internal/filedata/errors_and_messages.go | 5 - relay/filedata_actions_test.go | 2 +- relay/relay.go | 7 +- 11 files changed, 143 insertions(+), 158 deletions(-) diff --git a/config/config.go b/config/config.go index 9118ac41..bc02d915 100644 --- a/config/config.go +++ b/config/config.go @@ -83,6 +83,13 @@ var ( defaultRedisURL, _ = ct.NewOptURLAbsoluteFromString("redis://localhost:6379") //nolint:gochecknoglobals ) +const ( + // This minimum value was chosen not as a recommendation, but more to protect the host system from thousands of syscalls + + // the CPU time it takes to read the archive over and over again. It is somewhat arbitrary. + // It likely doesn't make sense to use an interval this frequent in production use-cases. + minimumFileDataSourceMonitoringInterval = 100 * time.Millisecond +) + // DefaultLoggers is the default logging configuration used by Relay. // // Output goes to stdout, except Error level which goes to stderr. Debug level is disabled. @@ -158,11 +165,12 @@ type AutoConfigConfig struct { // OfflineModeConfig contains configuration parameters for the offline/file data source feature. type OfflineModeConfig struct { - FileDataSource string `conf:"FILE_DATA_SOURCE"` - EnvDatastorePrefix string `conf:"ENV_DATASTORE_PREFIX"` - EnvDatastoreTableName string `conf:"ENV_DATASTORE_TABLE_NAME"` - EnvAllowedOrigin ct.OptStringList `conf:"ENV_ALLOWED_ORIGIN"` - EnvAllowedHeader ct.OptStringList `conf:"ENV_ALLOWED_HEADER"` + FileDataSource string `conf:"FILE_DATA_SOURCE"` + FileDataSourceMonitoringInterval ct.OptDuration `conf:"FILE_DATA_SOURCE_MONITORING_INTERVAL"` + EnvDatastorePrefix string `conf:"ENV_DATASTORE_PREFIX"` + EnvDatastoreTableName string `conf:"ENV_DATASTORE_TABLE_NAME"` + EnvAllowedOrigin ct.OptStringList `conf:"ENV_ALLOWED_ORIGIN"` + EnvAllowedHeader ct.OptStringList `conf:"ENV_ALLOWED_HEADER"` } // EventsConfig contains configuration parameters for proxying events. diff --git a/config/config_validation.go b/config/config_validation.go index adb68798..b126062b 100644 --- a/config/config_validation.go +++ b/config/config_validation.go @@ -18,11 +18,12 @@ var ( errOfflineModeWithEnvironments = errors.New("cannot configure specific environments if offline mode is enabled") errAutoConfWithoutDBDisambig = errors.New(`when using auto-configuration with database storage, database prefix (or,` + ` if using DynamoDB, table name) must be specified and must contain "` + AutoConfigEnvironmentIDPlaceholder + `"`) - errRedisURLWithHostAndPort = errors.New("please specify Redis URL or host/port, but not both") - errRedisBadHostname = errors.New("invalid Redis hostname") - errConsulTokenAndTokenFile = errors.New("Consul token must be specified as either an inline value or a file, but not both") //nolint:stylecheck - errAutoConfWithFilters = errors.New("cannot configure filters if auto-configuration is enabled") - errMissingProjKey = errors.New("when filters are configured, all environments must specify a 'projKey'") + errRedisURLWithHostAndPort = errors.New("please specify Redis URL or host/port, but not both") + errRedisBadHostname = errors.New("invalid Redis hostname") + errConsulTokenAndTokenFile = errors.New("Consul token must be specified as either an inline value or a file, but not both") //nolint:stylecheck + errAutoConfWithFilters = errors.New("cannot configure filters if auto-configuration is enabled") + errMissingProjKey = errors.New("when filters are configured, all environments must specify a 'projKey'") + errInvalidFileDataSourceMonitoringInterval = fmt.Errorf("file data source monitoring interval must be >= %s", minimumFileDataSourceMonitoringInterval) ) func errEnvironmentWithNoSDKKey(envName string) error { @@ -76,6 +77,7 @@ func ValidateConfig(c *Config, loggers ldlog.Loggers) error { validateConfigEnvironments(&result, c) validateConfigDatabases(&result, c, loggers) validateConfigFilters(&result, c) + validateOfflineMode(&result, c) return result.GetError() } @@ -122,7 +124,7 @@ func validateConfigEnvironments(result *ct.ValidationResult, c *Config) { } if c.OfflineMode.FileDataSource == "" { if c.OfflineMode.EnvDatastorePrefix != "" || c.OfflineMode.EnvDatastoreTableName != "" || - len(c.OfflineMode.EnvAllowedOrigin.Values()) != 0 || len(c.OfflineMode.EnvAllowedHeader.Values()) != 0 { + len(c.OfflineMode.EnvAllowedOrigin.Values()) != 0 || len(c.OfflineMode.EnvAllowedHeader.Values()) != 0 || c.OfflineMode.FileDataSourceMonitoringInterval.IsDefined() { result.AddError(nil, errOfflineModePropertiesWithNoFile) } } else { @@ -184,6 +186,16 @@ func validateConfigFilters(result *ct.ValidationResult, c *Config) { } } } + +func validateOfflineMode(result *ct.ValidationResult, c *Config) { + if c.OfflineMode.FileDataSourceMonitoringInterval.IsDefined() { + interval := c.OfflineMode.FileDataSourceMonitoringInterval.GetOrElse(0) + if interval < minimumFileDataSourceMonitoringInterval { + result.AddError(nil, errInvalidFileDataSourceMonitoringInterval) + } + } +} + func validateConfigDatabases(result *ct.ValidationResult, c *Config, loggers ldlog.Loggers) { normalizeRedisConfig(result, c) diff --git a/config/test_data_configs_invalid_test.go b/config/test_data_configs_invalid_test.go index abca958d..41cd917b 100644 --- a/config/test_data_configs_invalid_test.go +++ b/config/test_data_configs_invalid_test.go @@ -26,6 +26,9 @@ func makeInvalidConfigs() []testDataInvalidConfig { makeInvalidConfigOfflineModeAllowedHeaderWithNoFile(), makeInvalidConfigOfflineModePrefixWithNoFile(), makeInvalidConfigOfflineModeTableNameWithNoFile(), + makeInvalidConfigOfflineModeWithMonitoringInterval("0s"), + makeInvalidConfigOfflineModeWithMonitoringInterval("-1s"), + makeInvalidConfigOfflineModeWithMonitoringInterval("99ms"), makeInvalidConfigRedisInvalidHostname(), makeInvalidConfigRedisInvalidDockerPort(), makeInvalidConfigRedisConflictingParams(), @@ -240,6 +243,18 @@ EnvDatastoreTableName = table return c } +func makeInvalidConfigOfflineModeWithMonitoringInterval(interval string) testDataInvalidConfig { + c := testDataInvalidConfig{name: "offline mode table name with no file"} + c.fileError = errInvalidFileDataSourceMonitoringInterval.Error() + c.fileContent = ` +[OfflineMode] +fileDataSource = foo.tar.gz +fileDataSourceMonitoringInterval = ` + interval + ` + +` + return c +} + func makeInvalidConfigRedisInvalidHostname() testDataInvalidConfig { c := testDataInvalidConfig{name: "Redis - invalid hostname"} c.envVarsError = "invalid Redis hostname" diff --git a/config/test_data_configs_valid_test.go b/config/test_data_configs_valid_test.go index 9d35c4e7..a06bdcd0 100644 --- a/config/test_data_configs_valid_test.go +++ b/config/test_data_configs_valid_test.go @@ -65,7 +65,10 @@ func makeValidConfigs() []testDataValidConfig { makeValidConfigExplicitOldDefaultBaseURI(), makeValidConfigAutoConfig(), makeValidConfigAutoConfigWithDatabase(), - makeValidConfigFileData(), + makeValidConfigOfflineModeMinimal(), + makeValidConfigOfflineModeWithMonitoringInterval("100ms"), + makeValidConfigOfflineModeWithMonitoringInterval("1s"), + makeValidConfigOfflineModeWithMonitoringInterval("5m"), makeValidConfigRedisMinimal(), makeValidConfigRedisAll(), makeValidConfigRedisURL(), @@ -335,7 +338,7 @@ Enabled = true return c } -func makeValidConfigFileData() testDataValidConfig { +func makeValidConfigOfflineModeMinimal() testDataValidConfig { c := testDataValidConfig{name: "file data properties"} c.makeConfig = func(c *Config) { c.OfflineMode.FileDataSource = "my-file-path" @@ -350,6 +353,32 @@ FileDataSource = my-file-path return c } +func MustOptDurationFromString(duration string) ct.OptDuration { + opt, err := ct.NewOptDurationFromString(duration) + if err != nil { + panic(err) + } + return opt +} + +func makeValidConfigOfflineModeWithMonitoringInterval(interval string) testDataValidConfig { + c := testDataValidConfig{name: "file data properties"} + c.makeConfig = func(c *Config) { + c.OfflineMode.FileDataSource = "my-file-path" + c.OfflineMode.FileDataSourceMonitoringInterval = MustOptDurationFromString(interval) + } + c.envVars = map[string]string{ + "FILE_DATA_SOURCE": "my-file-path", + "FILE_DATA_SOURCE_MONITORING_INTERVAL": interval, + } + c.fileContent = ` +[OfflineMode] +FileDataSource = my-file-path +FileDataSourceMonitoringInterval = ` + interval + ` +` + return c +} + func makeValidConfigRedisMinimal() testDataValidConfig { c := testDataValidConfig{name: "Redis - minimal parameters"} c.makeConfig = func(c *Config) { diff --git a/docs/configuration.md b/docs/configuration.md index 74fa5ecb..f937df4b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -93,13 +93,14 @@ _(6)_ When using a database store, if there are multiple environments, it is nec This section is only applicable if [offline mode](https://docs.launchdarkly.com/home/advanced/relay-proxy-enterprise/offline) is enabled for your account. -| Property in file | Environment var | Type | Default | Description | -|--------------------------|----------------------------|:------:|:--------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `fileDataSource` | `FILE_DATA_SOURCE` | String | | Path to the offline mode data file that you have downloaded from LaunchDarkly. | -| `envDatastorePrefix` | `ENV_DATASTORE_PREFIX` | String | | If using a Redis, Consul, or DynamoDB store, this string will be added to all database keys to distinguish them from any other environments that are using the database. _(6)_ | -| `envDatastoreTableName ` | `ENV_DATASTORE_TABLE_NAME` | String | | If using a DynamoDB store, this specifies the table name. _(6)_ | -| `envAllowedOrigin` | `ENV_ALLOWED_ORIGIN` | URI | | If provided, adds CORS headers to prevent access from other domains. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_ORIGIN` variable, specify a comma-delimited list). | -| `envAllowedHeader` | `ENV_ALLOWED_HEADER` | String | | If provided, adds the specify headers to the list of accepted headers for CORS requests. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_HEADER` variable, specify a comma-delimited list). | +| Property in file | Environment var | Type | Default | Description | +|------------------------------------|----------------------------------------|:--------:|:--------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `fileDataSource` | `FILE_DATA_SOURCE` | String | | Path to the offline mode data file that you have downloaded from LaunchDarkly. | +| `fileDataSourceMonitoringInterval` | `FILE_DATA_SOURCE_MONITORING_INTERVAL` | Duration | `1s` | How often the file data source is checked for changes. Minimum is 100ms. To reduce computation and syscalls, raise the interval (for example, `5m` for every 5 minutes.) | +| `envDatastorePrefix` | `ENV_DATASTORE_PREFIX` | String | | If using a Redis, Consul, or DynamoDB store, this string will be added to all database keys to distinguish them from any other environments that are using the database. _(6)_ | +| `envDatastoreTableName ` | `ENV_DATASTORE_TABLE_NAME` | String | | If using a DynamoDB store, this specifies the table name. _(6)_ | +| `envAllowedOrigin` | `ENV_ALLOWED_ORIGIN` | URI | | If provided, adds CORS headers to prevent access from other domains. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_ORIGIN` variable, specify a comma-delimited list). | +| `envAllowedHeader` | `ENV_ALLOWED_HEADER` | String | | If provided, adds the specify headers to the list of accepted headers for CORS requests. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_HEADER` variable, specify a comma-delimited list). | Note that the last three properties have the same meanings and the same environment variables names as the corresponding properties in the `[AutoConfig]` section described above. It is not possible to use `[OfflineMode]` and `[AutoConfig]` at the same time. diff --git a/internal/filedata/archive_manager.go b/internal/filedata/archive_manager.go index 111c17b1..4d87f1ae 100644 --- a/internal/filedata/archive_manager.go +++ b/internal/filedata/archive_manager.go @@ -3,19 +3,18 @@ package filedata import ( "io" "os" - "path/filepath" "sync" "time" "github.com/launchdarkly/go-sdk-common/v3/ldlog" "github.com/launchdarkly/ld-relay/v8/config" - - "github.com/fsnotify/fsnotify" ) const ( - defaultRetryInterval = time.Millisecond * 500 - maxRetryDuration = time.Second * 2 + // This value was chosen as a default after switching from file-watcher event-based monitoring to simple polling. + // This idea is that polling should react fairly quickly to changes, just as the event-based system did to preserve + // any use-cases that relied on it. In practice, much longer intervals could likely be chosen. + defaultMonitoringInterval = 1 * time.Second ) // ArchiveManager manages the file data source. @@ -26,14 +25,13 @@ const ( // Relay provides an implementation of the UpdateHandler interface which will be called for all changes that // it needs to know about. type ArchiveManager struct { - filePath string - handler UpdateHandler - retryInterval time.Duration - lastKnownEnvs map[config.EnvironmentID]environmentMetadata - watcher *fsnotify.Watcher - loggers ldlog.Loggers - closeCh chan struct{} - closeOnce sync.Once + filePath string + monitoringInterval time.Duration + handler UpdateHandler + lastKnownEnvs map[config.EnvironmentID]environmentMetadata + loggers ldlog.Loggers + closeCh chan struct{} + closeOnce sync.Once } // ArchiveManagerInterface is an interface containing the public methods of ArchiveManager. This is used @@ -49,7 +47,7 @@ type ArchiveManagerInterface interface { func NewArchiveManager( filePath string, handler UpdateHandler, - retryInterval time.Duration, // zero = use the default; we set a nonzero brief interval in unit tests + monitoringInterval time.Duration, // zero = use the default; we set a nonzero brief interval in unit tests loggers ldlog.Loggers, ) (*ArchiveManager, error) { fileInfo, err := os.Stat(filePath) @@ -58,15 +56,15 @@ func NewArchiveManager( } am := &ArchiveManager{ - filePath: filePath, - handler: handler, - retryInterval: retryInterval, - lastKnownEnvs: make(map[config.EnvironmentID]environmentMetadata), - loggers: loggers, - closeCh: make(chan struct{}), + filePath: filePath, + handler: handler, + monitoringInterval: monitoringInterval, + lastKnownEnvs: make(map[config.EnvironmentID]environmentMetadata), + loggers: loggers, + closeCh: make(chan struct{}), } - if am.retryInterval == 0 { - am.retryInterval = defaultRetryInterval + if am.monitoringInterval == 0 { + am.monitoringInterval = defaultMonitoringInterval } am.loggers.SetPrefix("[FileDataSource]") @@ -76,16 +74,6 @@ func NewArchiveManager( } defer ar.Close() - watcher, err := fsnotify.NewWatcher() - if err != nil { - // COVERAGE: can't cause this condition in unit tests - unexpected failure of fsnotify package - return nil, errCreateArchiveManagerFailed(filePath, err) - } - if err := watcher.Add(filepath.Dir(filePath)); err != nil { - return nil, errCreateArchiveManagerFailed(filePath, err) // COVERAGE: see above - } - am.watcher = watcher - am.updatedArchive(ar) go am.monitorForChanges(fileInfo) @@ -100,106 +88,42 @@ func (am *ArchiveManager) Close() error { return nil } -func (am *ArchiveManager) monitorForChanges(originalFileInfo os.FileInfo) { - lastFileInfo := originalFileInfo - retryCh := make(chan struct{}) - pendingRetry := false - var firstRetryTime time.Time - var lastError error +func (am *ArchiveManager) monitorForChanges(original os.FileInfo) { + ticker := time.NewTicker(am.monitoringInterval) + defer ticker.Stop() - scheduleRetry := func() { - am.loggers.Infof(logMsgReloadWillRetry, am.retryInterval) - pendingRetry = true - if firstRetryTime.IsZero() { - firstRetryTime = time.Now() - } - time.AfterFunc(am.retryInterval, func() { - // Use non-blocking write because we never need to queue more than one retry signal - select { - case retryCh <- struct{}{}: - break - default: - break // COVERAGE: can't cause this condition in unit tests - } - }) - } + prevInfo := original - maybeReload := func() { - curFileInfo, err := os.Stat(am.filePath) - if err == nil { - if fileMayHaveChanged(curFileInfo, lastFileInfo) { - // If the file's mod time or size has changed, we will always try to reload. - firstRetryTime = time.Time{} - lastError = nil - am.loggers.Debugf("File info changed: old (size=%d, mtime=%s), new(size=%d, mtime=%s)", - lastFileInfo.Size(), lastFileInfo.ModTime(), curFileInfo.Size(), curFileInfo.ModTime()) - lastFileInfo = curFileInfo - ar, err := newArchiveReader(am.filePath) - if err != nil { - // A failure here might be a real failure, or it might be that the file is being copied - // over non-atomically so that we're seeing an invalid partial state. So we'll always - // retry at least once in this case. - am.loggers.Warnf(logMsgReloadError, err.Error()) - lastError = err - scheduleRetry() - return - } - am.loggers.Warnf(logMsgReloadedData, am.filePath) - am.updatedArchive(ar) - ar.Close() - return - } - am.loggers.Debugf("File has not changed (size=%d, mtime=%s)", curFileInfo.Size(), curFileInfo.ModTime()) - if lastError == nil { - // This was a spurious file watch notification - the file hasn't changed and we're not retrying - // after an error, so there's nothing to do - return - } - am.loggers.Warn(logMsgReloadUnchangedRetry) - } else if lastError == nil { - am.loggers.Warn(logMsgReloadFileNotFound) - lastError = err - } - // If we got here, then either the file was not found, or we triggered a delayed retry after - // an earlier error and the file has not changed since the last failed attempt. - // - // So there's no point in trying to reload it now, but it's still possible that there's a slow - // copy operation in progress, so we'll schedule another retry-- up to a limit. We won't rely on - // the file watching mechanism for this, because its granularity might be too large to detect - // consecutive changes that happen close together. - if firstRetryTime.IsZero() || time.Since(firstRetryTime) < maxRetryDuration { - scheduleRetry() - } else { - am.loggers.Errorf(logMsgReloadUnchangedNoMoreRetries, lastError) - } - } + am.loggers.Infof("Monitoring %s for changes (every %s) (size=%d, mtime=%s)", am.filePath, am.monitoringInterval, original.Size(), original.ModTime()) for { select { case <-am.closeCh: - _ = am.watcher.Close() return - - case event := <-am.watcher.Events: - am.loggers.Debugf("Got file watcher event: %+v", event) - // If we are already going to retry after a brief interval, then we can ignore any file watch - // events that are triggered before the retry. Some implementations of fsnotify can produce a - // burst of redundant events, and there's no point in trying to reload the file many times - // within our retry interval. - if pendingRetry { - am.loggers.Debug("Ignoring file watcher event because there is already a scheduled retry") - } else { - maybeReload() + case <-ticker.C: + nextInfo, err := os.Stat(am.filePath) + if err != nil { + if os.IsNotExist(err) { + am.loggers.Errorf("File %s not found", am.filePath) + } else { + am.loggers.Errorf("Error: %s", err) + } + continue } - - case <-retryCh: - // If needRetry is false, this is an obsolete signal - we've already successfully reloaded - if pendingRetry { - am.loggers.Debug("Got retry signal") - pendingRetry = false - maybeReload() + if fileMayHaveChanged(prevInfo, nextInfo) { + am.loggers.Infof("File %s has changed (size=%d, mtime=%s)", am.filePath, nextInfo.Size(), nextInfo.ModTime()) + reader, err := newArchiveReader(am.filePath) + if err != nil { + // A failure here might be a real failure, or it might be that the file is being copied + // over non-atomically so that we're seeing an invalid partial state. + am.loggers.Warnf(logMsgReloadError, err.Error()) + continue + } + am.loggers.Warnf("Reloaded data from %s", am.filePath) + am.updatedArchive(reader) + reader.Close() } else { - am.loggers.Debug("Ignoring obsolete retry signal") // COVERAGE: can't cause this condition in unit tests + am.loggers.Debugf("File %s has not changed (size=%d, mtime=%s)", am.filePath, nextInfo.Size(), nextInfo.ModTime()) } } } diff --git a/internal/filedata/archive_manager_test.go b/internal/filedata/archive_manager_test.go index 69daa02b..d8310489 100644 --- a/internal/filedata/archive_manager_test.go +++ b/internal/filedata/archive_manager_test.go @@ -90,7 +90,7 @@ func TestDefaultRetryInterval(t *testing.T) { require.NoError(t, err) defer archiveManager.Close() - assert.Equal(t, defaultRetryInterval, archiveManager.retryInterval) + assert.Equal(t, defaultMonitoringInterval, archiveManager.monitoringInterval) }) } @@ -186,7 +186,7 @@ func TestFileUpdatedWithInvalidDataAndDoesNotBecomeValid(t *testing.T) { writeMalformedArchive(p.filePath) - requireLogMessage(t, p.mockLog, ldlog.Error, "giving up until next change") + requireLogMessage(t, p.mockLog, ldlog.Warn, "Data file reload failed") p.requireNoMoreMessages() }) @@ -226,7 +226,7 @@ func TestFileDeletedAndThenRecreatedWithValidData(t *testing.T) { require.NoError(t, os.Remove(p.filePath)) - requireLogMessage(t, p.mockLog, ldlog.Warn, "file not found") + requireLogMessage(t, p.mockLog, ldlog.Error, "not found") testEnv1a := testEnv1.withMetadataChange().withSDKDataChange() writeArchive(t, p.filePath, false, nil, testEnv1a, testEnv2) @@ -246,7 +246,7 @@ func TestFileDeletedAndThenRecreatedWithInvalidDataAndThenValidData(t *testing.T require.NoError(t, os.Remove(p.filePath)) - requireLogMessage(t, p.mockLog, ldlog.Warn, "file not found") + requireLogMessage(t, p.mockLog, ldlog.Error, "not found") writeMalformedArchive(p.filePath) diff --git a/internal/filedata/archive_manager_test_base_test.go b/internal/filedata/archive_manager_test_base_test.go index 2a5f4a1d..073530eb 100644 --- a/internal/filedata/archive_manager_test_base_test.go +++ b/internal/filedata/archive_manager_test_base_test.go @@ -18,7 +18,7 @@ import ( ) const ( - testRetryInterval = time.Millisecond * 100 + testMonitoringInterval = time.Millisecond * 10 ) type archiveManagerTestParams struct { @@ -61,7 +61,7 @@ func archiveManagerTest(t *testing.T, setupFile func(filePath string), action fu archiveManager, err := NewArchiveManager( filePath, messageHandler, - testRetryInterval, + testMonitoringInterval, mockLog.Loggers, ) if archiveManager != nil { diff --git a/internal/filedata/errors_and_messages.go b/internal/filedata/errors_and_messages.go index bb6df33a..22c1432e 100644 --- a/internal/filedata/errors_and_messages.go +++ b/internal/filedata/errors_and_messages.go @@ -26,11 +26,6 @@ func errBadItemJSON(key, namespace string) error { func errCannotOpenArchiveFile(filePath string, err error) error { return fmt.Errorf("unable to read file data source %s: %w", filePath, err) } - -func errCreateArchiveManagerFailed(filePath string, err error) error { // COVERAGE: can't cause this condition in unit tests - return fmt.Errorf("unable to initialize archive manager for %q: %w", filePath, err) -} - func errChecksumDoesNotMatch(expected, actual string) error { return fmt.Errorf("checksum of environments did not match: expected %q, got %q", expected, actual) } diff --git a/relay/filedata_actions_test.go b/relay/filedata_actions_test.go index 7064a174..221239ab 100644 --- a/relay/filedata_actions_test.go +++ b/relay/filedata_actions_test.go @@ -73,7 +73,7 @@ func offlineModeTest( relay, err := newRelayInternal(config, relayInternalOptions{ loggers: mockLog.Loggers, clientFactory: testclient.RealLDClientFactoryWithChannel(true, clientsCreatedCh), - archiveManagerFactory: func(filename string, handler filedata.UpdateHandler, loggers ldlog.Loggers) ( + archiveManagerFactory: func(filename string, monitoringInterval time.Duration, handler filedata.UpdateHandler, loggers ldlog.Loggers) ( filedata.ArchiveManagerInterface, error) { p.updateHandler = handler return stubArchiveManager{}, nil diff --git a/relay/relay.go b/relay/relay.go index 5922d72b..3f47bf21 100644 --- a/relay/relay.go +++ b/relay/relay.go @@ -81,7 +81,7 @@ type ClientFactoryFunc func(sdkKey config.SDKKey, config ld.Config) (*ld.LDClien type relayInternalOptions struct { loggers ldlog.Loggers clientFactory sdks.ClientFactoryFunc - archiveManagerFactory func(string, filedata.UpdateHandler, ldlog.Loggers) (filedata.ArchiveManagerInterface, error) + archiveManagerFactory func(path string, monitoringInterval time.Duration, environmentUpdates filedata.UpdateHandler, loggers ldlog.Loggers) (filedata.ArchiveManagerInterface, error) } // NewRelay creates a new Relay given a configuration and a method to create a client. @@ -226,6 +226,7 @@ func newRelayInternal(c config.Config, options relayInternalOptions) (*Relay, er } archiveManager, err := factory( c.OfflineMode.FileDataSource, + c.OfflineMode.FileDataSourceMonitoringInterval.GetOrElse(0), &relayFileDataActions{r: r}, loggers, ) @@ -290,9 +291,9 @@ func makeFilteredEnvironments(c *config.Config) map[string]*config.EnvConfig { return out } -func defaultArchiveManagerFactory(filePath string, handler filedata.UpdateHandler, loggers ldlog.Loggers) ( +func defaultArchiveManagerFactory(filePath string, monitoringInterval time.Duration, handler filedata.UpdateHandler, loggers ldlog.Loggers) ( filedata.ArchiveManagerInterface, error) { - am, err := filedata.NewArchiveManager(filePath, handler, 0, loggers) + am, err := filedata.NewArchiveManager(filePath, handler, monitoringInterval, loggers) return am, err }