-
Notifications
You must be signed in to change notification settings - Fork 44
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Improve options handling and add
--error-retry-base-interval (#245)
* Remove deprecated "http-timeout" and "http-error-retry" options Those two options have been deprecated 5 years ago. There is likely no reason to keep them around anymore. Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com> * Keep the CLI options while querying the stores in `info` Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com> * Correctly set the default option values and merge them with the config When loading a config file, we need to set the options to their default values. Otherwise, when merging it with the CLI options, we could end up with unexpected results. Additionally we need to distinguish between a CLI option that is set to its default value because it was not provided as a launch parameter or if it was explicitly set by the client. This is important because in `MergedWith()` we should prefer the CLI option only if it was explicitly set. Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com> * Add --error-retry-base-interval as a CLI option Setting the base interval between error retries is a useful option, especially when the Internet connection might be unstable. Given that `error-retry` can already be set via the CLI, by adding an option for `error-retry-base-interval` too we can avoid the need to force users to ship a config file when its only purpose was to only change this value. Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com> * Set a 500ms default value for error-retry-base-interval When the Internet connection is unstable, a few requests might fail. In those cases, by default, Desync retries 3 times before giving up. However, if the connection momentarily becomes unstable and you retry immediately without waiting, the chances of those attempts still failing are very high. With this commit we set a more reasonable 500ms of base wait interval to give the clients an higher chance of success. Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com> --------- Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com>
- Loading branch information
Showing
7 changed files
with
220 additions
and
46 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/require" | ||
| "os" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| const defaultErrorRetry = 3 | ||
| const DefaultErrorRetryBaseInterval = 500 * time.Millisecond | ||
|
|
||
| func newTestOptionsCommand(opt *cmdStoreOptions) *cobra.Command { | ||
| cmd := &cobra.Command{} | ||
|
|
||
| addStoreOptions(opt, cmd.Flags()) | ||
| return cmd | ||
| } | ||
|
|
||
| func TestErrorRetryOptions(t *testing.T) { | ||
| for _, test := range []struct { | ||
| name string | ||
| args []string | ||
| cfgFileContent []byte | ||
| errorRetryStoreHit int | ||
| errorRetryStoreMiss int | ||
| baseIntervalStoreHit time.Duration | ||
| baseIntervalStoreMiss time.Duration | ||
| }{ | ||
| {"Config with the error retry and base interval set", | ||
| []string{""}, | ||
| []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), | ||
| 20, defaultErrorRetry, 250000000, DefaultErrorRetryBaseInterval, | ||
| }, | ||
| {"Error retry and base interval via command line args", | ||
| []string{"--error-retry", "10", "--error-retry-base-interval", "1s"}, | ||
| []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), | ||
| 10, 10, 1000000000, 1000000000, | ||
| }, | ||
| {"Config without error retry nor base interval", | ||
| []string{""}, | ||
| []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), | ||
| defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, | ||
| }, | ||
| {"Config with default error retry and base interval", | ||
| []string{""}, | ||
| []byte(`{"store-options": {"/store/*/":{"error-retry": 3, "error-retry-base-interval": 500000000}}}`), | ||
| defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, | ||
| }, | ||
| {"Config that disables error retry and base interval", | ||
| []string{""}, | ||
| []byte(`{"store-options": {"/store/*/":{"error-retry": 0, "error-retry-base-interval": 0}}}`), | ||
| 0, defaultErrorRetry, 0, DefaultErrorRetryBaseInterval, | ||
| }, | ||
| {"Disables error retry and base interval via command line args", | ||
| []string{"--error-retry", "0", "--error-retry-base-interval", "0"}, | ||
| []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), | ||
| 0, 0, 0, 0, | ||
| }, | ||
| {"Force the default values via command line args", | ||
| []string{"--error-retry", "3", "--error-retry-base-interval", "500ms"}, | ||
| []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 750000000}}}`), | ||
| defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, | ||
| }, | ||
| } { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| f, err := os.CreateTemp("", "desync-options") | ||
| require.NoError(t, err) | ||
| defer os.Remove(f.Name()) | ||
| _, err = f.Write(test.cfgFileContent) | ||
| require.NoError(t, err) | ||
|
|
||
| // Set the global config file name | ||
| cfgFile = f.Name() | ||
|
|
||
| initConfig() | ||
|
|
||
| var cmdOpt cmdStoreOptions | ||
|
|
||
| cmd := newTestOptionsCommand(&cmdOpt) | ||
| cmd.SetArgs(test.args) | ||
|
|
||
| // Execute the mock command, to load the options provided in the launch arguments | ||
| _, err = cmd.ExecuteC() | ||
| require.NoError(t, err) | ||
|
|
||
| configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") | ||
| opt := cmdOpt.MergedWith(configOptions) | ||
| require.Equal(t, test.errorRetryStoreHit, opt.ErrorRetry) | ||
| require.Equal(t, test.baseIntervalStoreHit, opt.ErrorRetryBaseInterval) | ||
|
|
||
| configOptions, err = cfg.GetStoreOptionsFor("/missingStore") | ||
| opt = cmdOpt.MergedWith(configOptions) | ||
| require.NoError(t, err) | ||
| require.Equal(t, test.errorRetryStoreMiss, opt.ErrorRetry) | ||
| require.Equal(t, test.baseIntervalStoreMiss, opt.ErrorRetryBaseInterval) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestStringOptions(t *testing.T) { | ||
| for _, test := range []struct { | ||
| name string | ||
| args []string | ||
| cfgFileContent []byte | ||
| clientCertStoreHit string | ||
| clientCertStoreMiss string | ||
| clientKeyStoreHit string | ||
| clientKeyStoreMiss string | ||
| caCertStoreHit string | ||
| caCertStoreMiss string | ||
| }{ | ||
| {"Config with options set", | ||
| []string{""}, | ||
| []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), | ||
| "/foo", "", "/bar", "", "/baz", "", | ||
| }, | ||
| {"Configs set via command line args", | ||
| []string{"--client-cert", "/aa/bb", "--client-key", "/another", "--ca-cert", "/ca"}, | ||
| []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), | ||
| "/aa/bb", "/aa/bb", "/another", "/another", "/ca", "/ca", | ||
| }, | ||
| {"Config without any of those string options set", | ||
| []string{""}, | ||
| []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), | ||
| "", "", "", "", "", "", | ||
| }, | ||
| {"Disable values from CLI args", | ||
| []string{"--client-cert", "", "--client-key", "", "--ca-cert", ""}, | ||
| []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), | ||
| "", "", "", "", "", "", | ||
| }, | ||
| } { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| f, err := os.CreateTemp("", "desync-options") | ||
| require.NoError(t, err) | ||
| defer os.Remove(f.Name()) | ||
| _, err = f.Write(test.cfgFileContent) | ||
| require.NoError(t, err) | ||
|
|
||
| // Set the global config file name | ||
| cfgFile = f.Name() | ||
|
|
||
| initConfig() | ||
|
|
||
| var cmdOpt cmdStoreOptions | ||
|
|
||
| cmd := newTestOptionsCommand(&cmdOpt) | ||
| cmd.SetArgs(test.args) | ||
|
|
||
| // Execute the mock command, to load the options provided in the launch arguments | ||
| _, err = cmd.ExecuteC() | ||
| require.NoError(t, err) | ||
|
|
||
| configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") | ||
| opt := cmdOpt.MergedWith(configOptions) | ||
| require.Equal(t, test.clientCertStoreHit, opt.ClientCert) | ||
| require.Equal(t, test.clientKeyStoreHit, opt.ClientKey) | ||
| require.Equal(t, test.caCertStoreHit, opt.CACert) | ||
|
|
||
| configOptions, err = cfg.GetStoreOptionsFor("/missingStore") | ||
| opt = cmdOpt.MergedWith(configOptions) | ||
| require.NoError(t, err) | ||
| require.Equal(t, test.clientCertStoreMiss, opt.ClientCert) | ||
| require.Equal(t, test.clientKeyStoreMiss, opt.ClientKey) | ||
| require.Equal(t, test.caCertStoreMiss, opt.CACert) | ||
| }) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters