refactor(object_store): refactor timeout and retry of object store interface #16231

Li0k · 2024-04-10T08:00:08Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Prior to this PR, Object Store could only configure the total timeout for some operations and put retry behavior on backend execution. This design is not flexible. We configure a more conservative timeout for each operation by default, which may extend the system recovery time in some scenarios. Moreover, the total timeout is not intuitive for users.

Therefore, this PR refactors the retry and timeout logic of the Object Store

Introduced separate timeout and retry configuration for each operation
Remove the retry logic of object store backend
Uniformly handle retry and timeout by MonitredObjectStore

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Introduce new timeout and retry configurations for all object store interfaces. The new configurations are located at [storage.object_store.retry] refer to the following

[storage.object_store.retry]
req_backoff_interval_ms = 1000
req_backoff_max_delay_ms = 10000
req_backoff_factor = 2
upload_attempt_timeout_ms = 8000
upload_retry_attempts = 3
streaming_upload_attempt_timeout_ms = 5000
streaming_upload_retry_attempts = 3

Deprecate ambiguous timeout configuration

[storage.object_store]
object_store_streaming_read_timeout_ms = 480000
object_store_streaming_upload_timeout_ms = 480000
object_store_upload_timeout_ms = 480000
object_store_read_timeout_ms = 480000

[storage.object_store.s3]
object_store_req_retry_interval_ms = 20
object_store_req_retry_max_delay_ms = 10000
object_store_req_retry_max_attempts = 8

TennyZhuang · 2024-04-10T09:04:23Z

What's iterface?

hzxa21 · 2024-04-11T06:51:57Z

src/object_store/src/object/mod.rs

+            || async {
+                let future = async {
+                    self.inner
+                        .upload(path, obj.clone())


Is the clone here necessary?

The parameter requirement must be a FnMut. How do we bypass clone?

hzxa21 · 2024-04-11T06:53:46Z

src/object_store/src/object/mod.rs

+            || async {
+                let future = async {
+                    self.inner
+                        .read(path, range.clone())


Is the clone here necessary?

hzxa21 · 2024-04-11T06:54:12Z

src/object_store/src/object/mod.rs

+            || async {
+                let future = async {
+                    self.inner
+                        .streaming_read(path, range.clone())


Is the clone here necessary?

src/object_store/src/object/s3.rs

src/object_store/src/object/error.rs

…nto li0k/storage_object_retry

src/common/src/config.rs

MrCroxx · 2024-04-15T08:40:51Z

src/object_store/src/object/error.rs

-    S3(#[source] BoxedError),
+    #[error("s3 error: {inner}")]
+    S3 {
+        // TODO: remove this after switch s3 backend to opendal


0.0 Why? Any infomation?

we can move to opendal::Error after switching to opendal

MrCroxx · 2024-04-15T08:41:55Z

src/object_store/src/object/mod.rs

+            || async {
+                let future = async {
+                    self.inner
+                        .upload(path, obj.clone())


src/object_store/src/object/mod.rs

src/object_store/src/object/opendal_engine/fs.rs

src/object_store/src/object/mod.rs

Li0k · 2024-05-07T07:08:08Z

https://grafana.test.risingwave-cloud.xyz/d/EpkBw5W4k/risingwave-dev-dashboard?from=1715009629513&orgId=1&to=1715050288336&var-component=All&var-datasource=P2453400D1763B4D9&var-instance=benchmark-risingwave&var-namespace=usrlngvty-20240506-143453&var-pod=All&var-table=All

…nto li0k/storage_object_retry

hzxa21 · 2024-05-07T13:38:19Z

grafana/risingwave-user-dashboard.dashboard.py

@@ -648,7 +648,7 @@ def section_storage(outer_panels):
                            [50, 99],
                        ),
                        panels.target(
-                            f"sum by(le, {COMPONENT_LABEL}) (rate({metric('state_store_sync_size_sum')}[$__rate_interval])) / sum by(le, {COMPONENT_LABEL}) (rate({metric('state_store_sync_size_count')}[$__rate_interval])) > 0",
+                            f"sum by(le, {COMPONENT_LABEL}) (rate({metric('state_store_sync_size_sum')}[$__rate_interval])) / sum by(le, {COMPONENT_LABEL}) (rate({metric('state_store_sync_size_count')}[$__rate_interval]))",


I think the conflict is not resolved correctly here in this file. > 0 and >= 0 are added to avoid showing NaN on division by zero.

hzxa21 · 2024-05-07T13:38:51Z

grafana/risingwave-dev-dashboard.dashboard.py

@@ -82,7 +82,7 @@ def section_cluster_node(outer_panels):
                            % (COMPONENT_LABEL, NODE_LABEL),
                        ),
                        panels.target(
-                            f"sum(rate({metric('process_cpu_seconds_total')}[$__rate_interval])) by ({COMPONENT_LABEL}, {NODE_LABEL}) / avg({metric('process_cpu_core_num')}) by ({COMPONENT_LABEL}, {NODE_LABEL}) > 0",
+                            f"sum(rate({metric('process_cpu_seconds_total')}[$__rate_interval])) by ({COMPONENT_LABEL}, {NODE_LABEL}) / avg({metric('process_cpu_core_num')}) by ({COMPONENT_LABEL}, {NODE_LABEL})",


hzxa21 · 2024-05-07T13:41:33Z

src/object_store/src/object/mod.rs

-                        "streaming_upload write_bytes timeout",
-                    ))
+                    Err(ObjectError::timeout(format!(
+                        "{} timeout",


This comment is not resolved.

…nto li0k/storage_object_retry

hzxa21 · 2024-05-08T07:46:01Z

src/object_store/src/object/mod.rs

+                .await
+                .unwrap_or_else(|_| {
+                    Err(ObjectError::timeout(format!(
+                        "{}_attempt_timeout_ms {:?} {}_retry_attempts {:?}",


"retry attempts exhausted for {}. Please modify {}_attempt_timeout_ms (current=...) and {}_retry_attempts (current=...) under [storage.object_store.retry] in the config accordingly if needed."

…nto li0k/storage_object_retry

hzxa21

LGTM. Thanks for the efforts!

…terface (#16231)

refactor(object_store): refactor timeout and retry of iterface

44ff6ff

github-actions bot added the type/refactor label Apr 10, 2024

refactor(object_storage): aws-sdk-s3 retry logic

faafbd1

hzxa21 reviewed Apr 11, 2024

View reviewed changes

Li0k added 2 commits April 11, 2024 15:49

refactor(object_store): refactor timeout and retry_attempts config

b8e252b

feat(object_store): remove RetryLayer of OpenDal backend

bbd7955

Li0k changed the title ~~refactor(object_store): refactor timeout and retry of iterface~~ refactor(object_store): refactor timeout and retry of object store interface Apr 11, 2024

Li0k added 7 commits April 11, 2024 16:10

fix(object_store): fix compile

f59f6e3

feat(object_store): retry for streming_read and streaming_upload

82ab21d

feat(storage): set s3 error should_retry

f7f3034

refactor(storage): deprecate total_timeout of interface

0a9a286

fix(object_store): fix retry metrics

0855678

fix(object_store): fix check

0510a5a

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

d14d684

…nto li0k/storage_object_retry

Li0k marked this pull request as ready for review April 12, 2024 11:03

fix(object_store): fix compile

171456f

Li0k requested review from Little-Wallace and MrCroxx April 15, 2024 08:06

Li0k added 3 commits April 15, 2024 16:23

refactor(object_store): remove some code

a05443a

refactor(object_store): remove retry config of aws-sdk

7e8014b

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

36b2df7

…nto li0k/storage_object_retry

MrCroxx reviewed Apr 15, 2024

View reviewed changes

Li0k added 2 commits April 16, 2024 16:55

fix(storage): fix ut

4d5cc57

fix(object_store): address comments

3327727

github-actions bot added the ci/run-e2e-single-node-tests label Apr 16, 2024

Li0k added 3 commits April 16, 2024 19:13

fix(object_store): fix check

3268f89

refactor(storage): refactor

ef3a0cc

fix(object_store): fix dashboard

db288c2

Li0k requested a review from wenym1 April 17, 2024 08:21

fix(grafana): fix grafana

4065164

Li0k added 3 commits May 7, 2024 15:09

fix(object_store): update example

cf28941

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

0f02386

…nto li0k/storage_object_retry

fix(grafana): upgrade grafana

70fdccb

hzxa21 reviewed May 7, 2024

View reviewed changes

Li0k added 5 commits May 8, 2024 14:22

fix(object_store): address comments

5f1b9b7

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

af51ace

…nto li0k/storage_object_retry

fix(grafana): fix grafana

e4d03e0

fix(grafana): fix grafana

a299ab1

fix(grafana): remove unused metrics

bdb0c93

hzxa21 reviewed May 8, 2024

View reviewed changes

Li0k added 2 commits May 8, 2024 16:19

fix(object_store): fix log

e7d7aca

Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…

fe85b40

…nto li0k/storage_object_retry

hzxa21 approved these changes May 8, 2024

View reviewed changes

Li0k added this pull request to the merge queue May 8, 2024

Li0k added the need-cherry-pick-release-1.9 label May 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 8, 2024

Li0k added this pull request to the merge queue May 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 8, 2024

Li0k added this pull request to the merge queue May 8, 2024

Merged via the queue into main with commit 2929fb8 May 8, 2024
32 of 33 checks passed

Li0k deleted the li0k/storage_object_retry branch May 8, 2024 18:00

neverchanje mentioned this pull request May 8, 2024

Document: refactor(object_store): refactor timeout and retry of object store interface risingwavelabs/risingwave-docs#2147

Closed

github-actions bot mentioned this pull request May 8, 2024

cherrypick refactor(object_store): refactor timeout and retry of object store interface (#16231) to branch release-1.9 #16646

Closed

Li0k added a commit that referenced this pull request May 9, 2024

refactor(object_store): refactor timeout and retry of object store in…

d77a9ac

…terface (#16231)

kwannoel mentioned this pull request May 10, 2024

Bug: Backfill tests broke due to read failed error=Timeout error: Retry attempts exhausted for read. Please modify read_attempt_timeout_ms (current=8000) #16675

Closed

BugenZhao mentioned this pull request May 21, 2024

risingwave 1.9.1 risingwavelabs/homebrew-risingwave#38

Closed

1 task

hzxa21 mentioned this pull request Jun 13, 2024

unify hummock s3 retry and timeout interface #13843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(object_store): refactor timeout and retry of object store interface #16231

refactor(object_store): refactor timeout and retry of object store interface #16231

Li0k commented Apr 10, 2024 •

edited

Loading

TennyZhuang commented Apr 10, 2024

hzxa21 Apr 11, 2024

MrCroxx Apr 15, 2024

Li0k Apr 16, 2024

hzxa21 Apr 11, 2024

hzxa21 Apr 11, 2024

MrCroxx Apr 15, 2024

Li0k Apr 16, 2024

MrCroxx Apr 15, 2024

Li0k commented May 7, 2024

hzxa21 May 7, 2024

hzxa21 May 7, 2024

hzxa21 May 7, 2024

hzxa21 May 8, 2024

hzxa21 left a comment

refactor(object_store): refactor timeout and retry of object store interface #16231

refactor(object_store): refactor timeout and retry of object store interface #16231

Conversation

Li0k commented Apr 10, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

TennyZhuang commented Apr 10, 2024

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

Li0k commented May 7, 2024

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

hzxa21 left a comment

Choose a reason for hiding this comment

Li0k commented Apr 10, 2024 •

edited

Loading