7705 retryable http client #7723

eladlachmi · 2024-05-05T18:36:57Z

Change Description

Background

See the associated issue for context.

New Feature

This PR creates a new HTTP client for lakectl, based on go-retryablehttp. It supports retries with a custom retry policy tailored for lakeFS and an exponential backoff retry strategy. Its constructor func includes several parameters to tune the retry max count and intervals and the option to provide a custom http.Transport. For seamless integration, the constructor returns a standard *http.Client.

Testing Details

This PR includes a suite of tests for the custom retry policy. The retry mechanism was tested using the abuse command with a playground installation, a modified DDB table, and a small number of RCUs/WCUs.

The abuse command:

./lakectl abuse random-write  lakefs://sample-repo/main --parallelism 100 --prefix abuse-3/

Without retries, this command fails with HTTP 500 errors. With retires it retries the failures and completes successfully.

Thanks @itaiad200 for the abuse command ✌🏻

github-actions · 2024-05-05T18:38:06Z

🎊 PR Preview 376e69b has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-7723.surge.sh

🕐 Build time: 0.014s

_{🤖 By surge-preview}

github-actions · 2024-05-05T18:42:06Z

E2E Test Results - DynamoDB Local - Local Block Adapter

guy-har

Approving due to urgency, there are no blocking comments, but please do consider them.

guy-har · 2024-05-06T06:31:15Z

docs/reference/cli.md

@@ -87,7 +87,7 @@ lakectl [flags]
 {:.no_toc}

 ```
-      --base-uri string      base URI used for lakeFS address parse
+      --base-uri string      base URI used for lakeFS address parse (default "https://treeverse-proper-boar-yjz5us.us-east-1.lakefscloud.ninja/api/v1")


What does this have to do lakeFS?

Oops... that wasn't supposed to be here...

guy-har · 2024-05-06T06:31:57Z

cmd/lakectl/cmd/root.go

@@ -521,6 +538,10 @@ func initConfig() {
 	// set defaults
 	viper.SetDefault("metastore.hive.db_location_uri", "file:/user/hive/warehouse/")
 	viper.SetDefault("server.endpoint_url", "http://127.0.0.1:8000")
+	viper.SetDefault("server.retries.enabled", true)


Isn't the number of retries enough to decide if we have retries or not?

It is, but I'd prefer to be explicit if it doesn't cost me anything.

Doesn't cost you, but does cost the user understanding
What happens if I don't want reties and I change retries to 0 because that's what I'm used to, what will happen here?

guy-har · 2024-05-06T06:34:44Z

cmd/lakectl/cmd/root.go

+	if !cfg.Server.Retries.Enabled {
+		httpClient = &http.Client{
+			Transport: transport,
+		}
+	} else {
+		httpClient = NewRetryClient(cfg.Server.Retries.MaxRetries,
+			cfg.Server.Retries.MinWaitInterval, cfg.Server.Retries.MaxWaitInterval, transport)


nit: but I would expect the newRetryClient to get the httpClient and not the transport, that we can initiate the client regardless and then just wrap it with the retry client if needed

guy-har · 2024-05-06T06:42:22Z

cmd/lakectl/cmd/retry_client.go

+	retryClient.RetryMax = maxRetries
+	retryClient.RetryWaitMin = retryWaitMin
+	retryClient.RetryWaitMax = retryWaitMax
+	retryClient.CheckRetry = lakectlRetryPolicy


I still believe the default is good enough for us

guy-har · 2024-05-06T06:47:32Z

cmd/lakectl/cmd/retry_client.go

+func NewRetryClient(maxRetries int, retryWaitMin, retryWaitMax time.Duration, transport *http.Transport) *http.Client {
+	retryClient := retryablehttp.NewClient()
+	if transport != nil {
+		retryClient.HTTPClient.Transport = transport
+	}


I still believe that a simple function in cmd/root.go will do. Also cmd is not the correct directory for this, I would expect it to be somewhere more generic, such as pkg/httputil

guy-har · 2024-05-06T06:51:09Z

cmd/lakectl/cmd/retry_client.go

+		retryClient.HTTPClient.Transport = transport
+	}
+	retryClient.Logger = nil
+	retryClient.RetryMax = maxRetries


aws cli, uses maxAttemps IMO it's clear, In max retries I never remember if the first try counts

I'm fine either way, but done.

guy-har · 2024-05-06T06:51:50Z

cmd/lakectl/cmd/root.go

+			MinWaitInterval time.Duration `mapstructure:"min_wait_interval"`
+			MaxWaitInterval time.Duration `mapstructure:"max_wait_interval"`


Can you please document this two

Added inline comments. If you can point me to the documentation, I'll add it there.

I guess a section here mentioning retries would be appropriate..

itaiad200

Looks great! request-changes-worthy-comments:

Lack of docs for the new values
I think we should start with something that is less configureable.

docs/reference/cli.md

itaiad200 · 2024-05-06T06:42:36Z

cmd/lakectl/cmd/root.go

+			Enabled         bool          `mapstructure:"enabled"`
+			MaxRetries      int           `mapstructure:"max_retries"`
+			MinWaitInterval time.Duration `mapstructure:"min_wait_interval"`
+			MaxWaitInterval time.Duration `mapstructure:"max_wait_interval"`


These new values should go to the cli reference doc page.
I also suggest to start leaner, maybe just Enabled & MaxRetries to kick it off? Maybe even just Enabled?
I believe the defaults you set would be good enough for almost everyone. I don't feel like we have enough information to determine users need to configure intervals for now.. We can always add more custom config later, removing it is the hard part..

I don't see the point of providing fewer configuration options. We lose the flexibility to configure for optimal results, and this way, we'll never get feedback on which settings work for different use cases.

Because it's adding scope and friction. Instead of having "retry mechanism" for lakectl, we have "configurable retry mechanism". The gap isn't big coding-wise, I agree. But now I have to ask annoying questions regarding usage, like MaxRetries non-positive, MaxWaitInterval smaller than MinWaitInterval, why choose these values and not TotalWaitTime, etc. Without someone requesting something specific we're guessing what they need here, and then changing it might be a breaking change in the cli configuration.

cmd/lakectl/cmd/retry_client.go

itaiad200 · 2024-05-06T06:46:19Z

cmd/lakectl/cmd/root.go

@@ -140,6 +147,10 @@ const (
 	allowEmptyMsgFlagName = "allow-empty-message"
 	fmtErrEmptyMsg        = `commit with no message without specifying the "--allow-empty-message" flag`
 	metaFlagName          = "meta"
+
+	defaultMaxRetries       = 3
+	defaultMaxRetryInterval = 1 * time.Second


Too low IMO

Suggested change

defaultMaxRetryInterval = 1 * time.Second

defaultMaxRetryInterval = 5 * time.Second

I used a different value, but raised the max interval.

cmd/lakectl/cmd/retry_client.go

arielshaqed

No comments other than not understanding the motivation for the chosen default values.

arielshaqed · 2024-05-06T09:22:03Z

cmd/lakectl/cmd/root.go

+	defaultMaxRetries       = 3
+	defaultMaxRetryInterval = 1 * time.Second
+	defaultMinRetryInterval = 200 * time.Millisecond


I don't understand these numbers. Suppose the HTTP server never sends a "retry after" header (I don't think lakeFS does). Then we have 3 retries, starting with 0.2 s and going up to 1 second. But 3 retries will sleep for 0.2s and then for 0.4s. When does this max retry interval ever come into play?

If we had 6 retries, then IIUC our wait sequence would be (in seconds) 0.2, 0.4, 0.8, 1.0, 1.0, 1.0.

For example, AFAICT this is the default configuration.

If we had 6 retries, then IIUC our wait sequence would be (in seconds) 0.2, 0.4, 0.8, 1.0, 1.0, 1.0

Yes, that's accurate. I'm not sure why this is a problem, but I don't mind using the package defaults instead.

arielshaqed

Thanks!

itaiad200

LGTM
I still prefer that we start with leaner configuration but won't block on it..

eladlachmi · 2024-05-06T12:33:43Z

I want to take this opportunity to say that @arielshaqed was right! I should unwrap the error 😊

* lakectl - Support Retries

eladlachmi added 4 commits May 1, 2024 18:15

lakectl - HTTP Client Supporting Retries

60c0049

Retryable http client

d74e7b8

Disable logger

c54fd60

Remove manual test

376e69b

eladlachmi added area/lakectl Issues related to lakeFS' command line interface (lakectl) include-changelog PR description should be included in next release changelog labels May 5, 2024

eladlachmi requested review from arielshaqed, itaiad200 and guy-har May 5, 2024 18:36

eladlachmi self-assigned this May 5, 2024

guy-har approved these changes May 6, 2024

View reviewed changes

guy-har reviewed May 6, 2024

View reviewed changes

itaiad200 requested changes May 6, 2024

View reviewed changes

arielshaqed reviewed May 6, 2024

View reviewed changes

eladlachmi added 6 commits May 6, 2024 13:50

Review fixes

90519e7

Unwrap url.Error

997f90f

Fix test

5c46466

Fix comment

d7b0397

Return unwraped error

3fa69aa

Fix tests

13dbdee

eladlachmi requested review from arielshaqed and itaiad200 May 6, 2024 12:12

arielshaqed reviewed May 6, 2024

View reviewed changes

itaiad200 approved these changes May 6, 2024

View reviewed changes

eladlachmi merged commit 3821ff0 into master May 6, 2024
35 checks passed

eladlachmi deleted the 7705-retryable-http-client branch May 6, 2024 12:44

emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024

7705 retryable http client (treeverse#7723)

6068806

* lakectl - Support Retries

oliverdain mentioned this pull request Jun 4, 2024

Add a catch/retry mechanism to lakectl local operations #7651

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7705 retryable http client #7723

7705 retryable http client #7723

eladlachmi commented May 5, 2024

github-actions bot commented May 5, 2024 •

edited

github-actions bot commented May 5, 2024

guy-har left a comment

guy-har May 6, 2024

eladlachmi May 6, 2024

guy-har May 6, 2024

eladlachmi May 6, 2024

guy-har May 6, 2024

guy-har May 6, 2024

guy-har May 6, 2024

guy-har May 6, 2024

guy-har May 6, 2024

eladlachmi May 6, 2024

guy-har May 6, 2024

eladlachmi May 6, 2024

itaiad200 May 6, 2024

itaiad200 left a comment

itaiad200 May 6, 2024

eladlachmi May 6, 2024

itaiad200 May 6, 2024

itaiad200 May 6, 2024

eladlachmi May 6, 2024

arielshaqed left a comment

arielshaqed May 6, 2024

eladlachmi May 6, 2024

eladlachmi May 6, 2024

arielshaqed left a comment

itaiad200 left a comment

eladlachmi commented May 6, 2024

		MinWaitInterval time.Duration `mapstructure:"min_wait_interval"`
		MaxWaitInterval time.Duration `mapstructure:"max_wait_interval"`

	defaultMaxRetryInterval = 1 * time.Second
	defaultMaxRetryInterval = 5 * time.Second

7705 retryable http client #7723

7705 retryable http client #7723

Conversation

eladlachmi commented May 5, 2024

Change Description

Background

New Feature

Testing Details

github-actions bot commented May 5, 2024 • edited

github-actions bot commented May 5, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

guy-har left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

eladlachmi commented May 6, 2024

github-actions bot commented May 5, 2024 •

edited