[querier] s3: add getObject retry #4453

liguozhong · 2021-10-11T09:22:22Z

fix #4452
this pr fix issues/4452

liguozhong · 2021-10-11T09:48:55Z

doing

liguozhong · 2021-10-11T14:22:51Z

cyriltovena · 2021-10-12T14:36:34Z

pkg/storage/chunk/aws/s3_storage_client.go

@@ -115,6 +117,10 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	f.BoolVar(&cfg.HTTPConfig.InsecureSkipVerify, prefix+"s3.http.insecure-skip-verify", false, "Set to true to skip verifying the certificate chain and hostname.")
 	f.StringVar(&cfg.HTTPConfig.CAFile, prefix+"s3.http.ca-file", "", "Path to the trusted CA file that signed the SSL certificate of the S3 endpoint.")
 	f.StringVar(&cfg.SignatureVersion, prefix+"s3.signature-version", SignatureVersionV4, fmt.Sprintf("The signature version to use for authenticating against S3. Supported values are: %s.", strings.Join(supportedSignatureVersions, ", ")))
+	f.DurationVar(&cfg.BackoffConfig.MinBackoff, "s3.min-backoff", 100*time.Millisecond, "Minimum backoff time")
+	f.DurationVar(&cfg.BackoffConfig.MaxBackoff, "s3.max-backoff", 50*time.Second, "Maximum backoff time")
+	f.IntVar(&cfg.BackoffConfig.MaxRetries, "s3.max-retries", 20, "Maximum number of times to retry an operation")


Suggested change

f.IntVar(&cfg.BackoffConfig.MaxRetries, "s3.max-retries", 20, "Maximum number of times to retry an operation")

f.IntVar(&cfg.BackoffConfig.MaxRetries, "s3.max-retries", 5, "Maximum number of times to retry an operation")

cyriltovena · 2021-10-12T14:36:50Z

pkg/storage/chunk/aws/s3_storage_client.go

@@ -115,6 +117,10 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	f.BoolVar(&cfg.HTTPConfig.InsecureSkipVerify, prefix+"s3.http.insecure-skip-verify", false, "Set to true to skip verifying the certificate chain and hostname.")
 	f.StringVar(&cfg.HTTPConfig.CAFile, prefix+"s3.http.ca-file", "", "Path to the trusted CA file that signed the SSL certificate of the S3 endpoint.")
 	f.StringVar(&cfg.SignatureVersion, prefix+"s3.signature-version", SignatureVersionV4, fmt.Sprintf("The signature version to use for authenticating against S3. Supported values are: %s.", strings.Join(supportedSignatureVersions, ", ")))
+	f.DurationVar(&cfg.BackoffConfig.MinBackoff, "s3.min-backoff", 100*time.Millisecond, "Minimum backoff time")
+	f.DurationVar(&cfg.BackoffConfig.MaxBackoff, "s3.max-backoff", 50*time.Second, "Maximum backoff time")


Suggested change

f.DurationVar(&cfg.BackoffConfig.MaxBackoff, "s3.max-backoff", 50*time.Second, "Maximum backoff time")

f.DurationVar(&cfg.BackoffConfig.MaxBackoff, "s3.max-backoff", 3*time.Second, "Maximum backoff time")

cyriltovena

Looking good can you add more meat to the flag documentation, e.g related to s3 get operation.

liguozhong · 2021-10-12T16:15:36Z

Looking good can you add more meat to the flag documentation, e.g related to s3 get operation.

done

liguozhong · 2021-10-14T08:08:09Z

@cyriltovena review pr pls

pkg/storage/chunk/aws/s3_storage_client.go

DylanGuedes · 2021-10-14T12:19:53Z

Btw, you forgot to write the changes on the CHANGELOG and to document the new parameters in the docs located at docs/sources/configuration/_index.md.

Suggestion:

* [4453](https://github.com/grafana/loki/pull/4453) **liguozhong**: Loki: Implement retry to s3 chunk storage

pkg/storage/chunk/aws/s3_storage_client.go

liguozhong · 2021-10-14T13:12:56Z

Btw, you forgot to write the changes on the CHANGELOG and to document the new parameters in the docs located at docs/sources/configuration/_index.md.

Suggestion:
* [4453](https://github.com/grafana/loki/pull/4453) **liguozhong**: Loki: Implement retry to s3 chunk storage

thanks,done

docs/sources/configuration/_index.md

KMiller-Grafana

Docs change to correct spelling.

KMiller-Grafana · 2021-10-15T17:15:10Z

docs/sources/configuration/_index.md

+  # Configures backoff when s3 get Object.
+  backoff_config:
+    # Minimum delay when backing off.
+    # CLI flag: -s3.backoff-min-period
+    [min_period: <duration> | default = 100ms]
+
+    # The maximum delay when backing off.
+    # CLI flag: -s3.backoff-max-period
+    [max_period: <duration> | default = 3s]
+
+    # Number of times to backoff and retry before failing.


Suggested change

# Configures backoff when s3 get Object.

backoff_config:

# Minimum delay when backing off.

# CLI flag: -s3.backoff-min-period

[min_period: <duration> | default = 100ms]

# The maximum delay when backing off.

# CLI flag: -s3.backoff-max-period

[max_period: <duration> | default = 3s]

# Number of times to backoff and retry before failing.

# Configures back off when s3 get Object.

backoff_config:

# Minimum duration to back off.

# CLI flag: -s3.backoff-min-period

[min_period: <duration> | default = 100ms]

# The maximum duration to back off.

# CLI flag: -s3.backoff-max-period

[max_period: <duration> | default = 3s]

# Number of times to back off and retry before failing.

liguozhong · 2021-10-15T21:38:32Z

Docs change to correct spelling.

done ,thanks

KMiller-Grafana

Docs portion of this PR looks good to me now. Next time @liguozhong , you can just hit the 'commit suggestion' box to accept suggested changes. That would make it trivial for me to re-review.

liguozhong · 2021-10-19T02:25:47Z

Docs portion of this PR looks good to me now. Next time @liguozhong , you can just hit the 'commit suggestion' box to accept suggested changes. That would make it trivial for me to re-review.

👍 thanks

liguozhong · 2021-10-19T09:44:13Z

@DylanGuedes review pr pls

DylanGuedes · 2021-10-19T11:02:11Z

@DylanGuedes review pr pls

Do you mind having a look at this suggestion? I think that the check starting at line 344 is unnecessary after the latest changes.

Besides that, this looks solid, good job. Not sure if the maintainers would like to see some tests (although I checked the test files and looks like that function wasn't being tested so maybe they won't block you for that).

liguozhong · 2021-10-19T13:23:29Z

@DylanGuedes review pr pls

Do you mind having a look at this suggestion? I think that the check starting at line 344 is unnecessary after the latest changes.

Besides that, this looks solid, good job. Not sure if the maintainers would like to see some tests (although I checked the test files and looks like that function wasn't being tested so maybe they won't block you for that).

I'm not familiar with the'commit suggestion' box。
Code optimization has been done according to your suggestions。

DylanGuedes · 2021-10-19T13:37:32Z

pkg/storage/chunk/aws/s3_storage_client.go

+	err := ctx.Err()
+	for retries.Ongoing() {
+		if ctx.Err() != nil {
+			return nil, errors.Wrap(ctx.Err(), "ctx related error during s3 getObject")
+		}
+		err = instrument.CollectedRequest(ctx, "S3.GetObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {


Suggested change

err := ctx.Err()

for retries.Ongoing() {

if ctx.Err() != nil {

return nil, errors.Wrap(ctx.Err(), "ctx related error during s3 getObject")

}

err = instrument.CollectedRequest(ctx, "S3.GetObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {

for retries.Ongoing() {

if err := ctx.Err(); err != nil {

return nil, errors.Wrap(err, "ctx related error during s3 getObject")

}

err := instrument.CollectedRequest(ctx, "S3.GetObject", s3RequestDuration, instrument.ErrorCode, func(ctx context.Context) error {

DylanGuedes · 2021-10-19T13:39:04Z

@DylanGuedes review pr pls

Do you mind having a look at this suggestion? I think that the check starting at line 344 is unnecessary after the latest changes.
Besides that, this looks solid, good job. Not sure if the maintainers would like to see some tests (although I checked the test files and looks like that function wasn't being tested so maybe they won't block you for that).

I'm not familiar with the'commit suggestion' box。 Code optimization has been done according to your suggestions。

My bad, I didn't notice the change. I added a final one where I suggest moving the err assignment.

Anyway, LGTM!

liguozhong · 2021-10-19T16:11:12Z

@DylanGuedes review pr pls

Do you mind having a look at this suggestion? I think that the check starting at line 344 is unnecessary after the latest changes.
Besides that, this looks solid, good job. Not sure if the maintainers would like to see some tests (although I checked the test files and looks like that function wasn't being tested so maybe they won't block you for that).

I'm not familiar with the'commit suggestion' box。 Code optimization has been done according to your suggestions。

My bad, I didn't notice the change. I added a final one where I suggest moving the err assignment.

Anyway, LGTM!

sorry, I'm not familiar with the'commit suggestion' box。
I will try to learn this next time I submit a PR

owen-d

LGTM, thanks for your patience.

liguozhong · 2021-10-26T03:44:17Z

LGTM, thanks for your patience.

thanks

[querier] s3: add getObject retry

eae9311

liguozhong requested a review from a team as a code owner October 11, 2021 09:22

pull-request-size bot added the size/S label Oct 11, 2021

liguozhong closed this Oct 11, 2021

[querier] s3: add getObject retry

3a649f3

liguozhong reopened this Oct 11, 2021

pull-request-size bot added size/M and removed size/S labels Oct 11, 2021

liguozhong added 3 commits October 11, 2021 21:16

[querier] s3: add getObject retry grafana#4452

ba6b430

[querier] s3: add getObject retry

ece44a3

[querier] s3: add getObject retry grafana#4453

3d997cd

cyriltovena reviewed Oct 12, 2021

View reviewed changes

[querier] s3: add getObject retry

0d8dc80

DylanGuedes reviewed Oct 14, 2021

View reviewed changes

pkg/storage/chunk/aws/s3_storage_client.go Outdated Show resolved Hide resolved

[querier] s3: add getObject retry grafana#4453

64e475c

liguozhong requested a review from KMiller-Grafana as a code owner October 14, 2021 13:12

[querier] s3: add getObject retry grafana#4453

2d78414

DylanGuedes reviewed Oct 14, 2021

View reviewed changes

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved

liguozhong added 2 commits October 14, 2021 21:52

[querier] s3: add getObject retry grafana#4453

4a7bbe6

[querier] s3: add getObject retry grafana#4453

f971432

KMiller-Grafana suggested changes Oct 15, 2021

View reviewed changes

[querier] s3: add getObject retry

faf6585

KMiller-Grafana approved these changes Oct 18, 2021

View reviewed changes

DylanGuedes reviewed Oct 19, 2021

View reviewed changes

liguozhong requested a review from cyriltovena October 22, 2021 13:23

owen-d approved these changes Oct 25, 2021

View reviewed changes

Merge branch 'main' into s3_retry

fcd126d

owen-d requested review from dannykopping and slim-bean as code owners October 25, 2021 17:35

Merge branch 'main' into s3_retry

b3e726b

owen-d merged commit 6fcd02b into grafana:main Oct 25, 2021

JordanRushing mentioned this pull request Nov 10, 2021

Add retry with exponential backoff to S3ObjectClient.PutObject() #4732

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[querier] s3: add getObject retry #4453

[querier] s3: add getObject retry #4453

liguozhong commented Oct 11, 2021 •

edited

Loading

liguozhong commented Oct 11, 2021

liguozhong commented Oct 11, 2021

cyriltovena Oct 12, 2021

liguozhong Oct 12, 2021

cyriltovena Oct 12, 2021

liguozhong Oct 12, 2021

cyriltovena left a comment

liguozhong commented Oct 12, 2021

liguozhong commented Oct 14, 2021

DylanGuedes commented Oct 14, 2021 •

edited

Loading

liguozhong commented Oct 14, 2021

KMiller-Grafana left a comment

KMiller-Grafana Oct 15, 2021

liguozhong commented Oct 15, 2021

KMiller-Grafana left a comment

liguozhong commented Oct 19, 2021

liguozhong commented Oct 19, 2021

DylanGuedes commented Oct 19, 2021 •

edited

Loading

liguozhong commented Oct 19, 2021 •

edited

Loading

DylanGuedes Oct 19, 2021

DylanGuedes commented Oct 19, 2021

liguozhong commented Oct 19, 2021

owen-d left a comment

liguozhong commented Oct 26, 2021

	f.IntVar(&cfg.BackoffConfig.MaxRetries, "s3.max-retries", 20, "Maximum number of times to retry an operation")
	f.IntVar(&cfg.BackoffConfig.MaxRetries, "s3.max-retries", 5, "Maximum number of times to retry an operation")

	f.DurationVar(&cfg.BackoffConfig.MaxBackoff, "s3.max-backoff", 50*time.Second, "Maximum backoff time")
	f.DurationVar(&cfg.BackoffConfig.MaxBackoff, "s3.max-backoff", 3*time.Second, "Maximum backoff time")

[querier] s3: add getObject retry #4453

[querier] s3: add getObject retry #4453

Conversation

liguozhong commented Oct 11, 2021 • edited Loading

liguozhong commented Oct 11, 2021

liguozhong commented Oct 11, 2021

cyriltovena Oct 12, 2021

Choose a reason for hiding this comment

liguozhong Oct 12, 2021

Choose a reason for hiding this comment

cyriltovena Oct 12, 2021

Choose a reason for hiding this comment

liguozhong Oct 12, 2021

Choose a reason for hiding this comment

cyriltovena left a comment

Choose a reason for hiding this comment

liguozhong commented Oct 12, 2021

liguozhong commented Oct 14, 2021

DylanGuedes commented Oct 14, 2021 • edited Loading

liguozhong commented Oct 14, 2021

KMiller-Grafana left a comment

Choose a reason for hiding this comment

KMiller-Grafana Oct 15, 2021

Choose a reason for hiding this comment

liguozhong commented Oct 15, 2021

KMiller-Grafana left a comment

Choose a reason for hiding this comment

liguozhong commented Oct 19, 2021

liguozhong commented Oct 19, 2021

DylanGuedes commented Oct 19, 2021 • edited Loading

liguozhong commented Oct 19, 2021 • edited Loading

DylanGuedes Oct 19, 2021

Choose a reason for hiding this comment

DylanGuedes commented Oct 19, 2021

liguozhong commented Oct 19, 2021

owen-d left a comment

Choose a reason for hiding this comment

liguozhong commented Oct 26, 2021

liguozhong commented Oct 11, 2021 •

edited

Loading

DylanGuedes commented Oct 14, 2021 •

edited

Loading

DylanGuedes commented Oct 19, 2021 •

edited

Loading

liguozhong commented Oct 19, 2021 •

edited

Loading