feat(object store): introduce object prefix for opendal object store #16542

wcy-fdu · 2024-04-29T09:28:08Z

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

What's changed and what's your intention?

This PR introduce a new system params use_new_object_prefix_strategy to control whether split object prefix. In general,

if it's a new cluster, the use_new_object_prefix_strategy is set to true and all opendal object store will split prefix;
if it's an old cluster, the use_new_object_prefix_strategy is set to false and everything keep the same with before.

Thus, we can guarantee backward compatibility.

Once an origin customer find some issue with the performance and want to split prefix for his old cluster, it's required to execute a script to completely rename the data in the object store and modify the system params in some way.

So in a word, this pr will not affect the behavior of an already exist cluster.

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.

wcy-fdu · 2024-05-28T07:41:19Z

PTAL❤️cc @hzxa21 @Li0k

e2e_test/batch/catalog/pg_settings.slt.part

src/common/src/system_param/mod.rs

src/jni_core/src/hummock_iterator.rs

hzxa21 · 2024-05-30T03:20:12Z

src/meta/src/hummock/manager/mod.rs

+            if use_new_object_prefix_strategy {
+                match env.system_params_manager_impl_ref() {
+                    SystemParamsManagerImpl::Kv(mgr) => {
+                        mgr.set_param("use_new_object_prefix_strategy", Some("true".to_owned()))
+                            .await
+                            .unwrap();
+                    }
+                    SystemParamsManagerImpl::Sql(mgr) => {
+                        mgr.set_param("use_new_object_prefix_strategy", Some("true".to_owned()))
+                            .await
+                            .unwrap();
+                    }
+                };
+            } else {
+                match env.system_params_manager_impl_ref() {
+                    SystemParamsManagerImpl::Kv(mgr) => {
+                        mgr.set_param("use_new_object_prefix_strategy", Some("false".to_owned()))
+                            .await
+                            .unwrap();
+                    }
+                    SystemParamsManagerImpl::Sql(mgr) => {
+                        mgr.set_param("use_new_object_prefix_strategy", Some("false".to_owned()))
+                            .await
+                            .unwrap();
+                    }
+                };
+            }


I suggest we move the logic to MetaSrvEnv::new instead here becuase:

cluster_first_launch can be obtained directly there without interacting with object store and modifying write_exclusive_cluster_id

We can avoid making use_new_object_prefix_strategy mutable and instead pass the init value for use_new_object_prefix_strategy to init system params manager.

cluster_first_launch can be obtained directly there

For KV meta store, it is, but for SQL meta store, can we know whether it is the first launch here?

cc @yezizp2012 Can we also judge cluster_first_launch for SQL backend, just like KV backend?
Otherwise we need to modify this system variable later, but "Making the use_new_object_prefix_strategy mutable will be dangerous because user can techinically change it via ALTER in SQL and the after a restart, the whole cluster will crash."

After some offline discussion, for SQL backend I use MigratorTrait::get_applied_migrations() to check whether it's a new cluster:

if m20230908_072257_init is applied, it's an old cluster

if m20230908_072257_init is not applied, it's a new cluster.

Then we can make use_new_object_prefix_strategy immutable.

src/storage/benches/bench_compactor.rs

src/storage/benches/bench_multi_builder.rs

src/storage/hummock_test/src/bin/replay/main.rs

src/storage/src/hummock/iterator/test_utils.rs

src/tests/compaction_test/src/compaction_test_runner.rs

wcy-fdu · 2024-06-03T09:13:17Z

src/meta/src/manager/env.rs

@@ -416,11 +417,25 @@ impl MetaSrvEnv {
                    .await?
                    .map(|c| c.cluster_id.to_string().into())
                    .unwrap();
+                let migrations = Migrator::get_applied_migrations(&sql_meta_store.conn).await?;
+                let mut cluster_first_launch = true;


@yezizp2012 PTAL for this part, thanks!

We should check the status before calling Migrator::up. At this point all migrations has already been applied. Please add a helper function to get whether the cluster is first launched, together with some necessary comments.

We should check the status before calling Migrator::up. At this point all migrations has already been applied. Please add a helper function to get whether the cluster is first launched, together with some necessary comments.

+1. If we check get_applied_migrations here, it will contain "m20230908_072257_init" for sure.

Fixed, and since we explicitly specify that use_new_prefix_strategy should be passed in, there is no need to make any further judgment for ctl, right?

src/ctl/src/cmd_impl/hummock/sst_dump.rs

src/ctl/src/common/hummock_service.rs

hzxa21 · 2024-06-03T10:01:06Z

src/meta/src/manager/env.rs

@@ -416,11 +417,25 @@ impl MetaSrvEnv {
                    .await?
                    .map(|c| c.cluster_id.to_string().into())
                    .unwrap();
+                let migrations = Migrator::get_applied_migrations(&sql_meta_store.conn).await?;
+                let mut cluster_first_launch = true;


We should check the status before calling Migrator::up. At this point all migrations has already been applied. Please add a helper function to get whether the cluster is first launched, together with some necessary comments.

+1. If we check get_applied_migrations here, it will contain "m20230908_072257_init" for sure.

src/common/src/system_param/mod.rs

src/common/src/system_param/reader.rs

src/config/example.toml

src/meta/src/manager/env.rs

wcy-fdu · 2024-06-14T03:15:20Z

Backward compatibility tests pass.

**

**

src/meta/src/manager/env.rs

hzxa21

LGTM. Thanks for the PR!

gitguardian · 2024-06-17T09:06:05Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`6de4852`	ci/scripts/e2e-sink-test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

Li0k · 2024-06-18T06:20:15Z

src/meta/node/src/server.rs

@@ -408,13 +407,6 @@ pub async fn start_service_as_election_leader(
    mut svc_shutdown_rx: WatchReceiver<()>,
 ) -> MetaResult<()> {
    tracing::info!("Defining leader services");
-    if let MetaStoreImpl::Sql(sql_store) = &meta_store_impl {


why remove this code ?

add prefix for azblob

93ea4c4

github-actions bot added the type/feature label Apr 29, 2024

wcy-fdu requested a review from Li0k April 29, 2024 10:25

wcy-fdu changed the title ~~feat(object store): add prefix for azblob~~ feat(object store): devide prefix for azblob object store Apr 30, 2024

wcy-fdu added 2 commits May 23, 2024 18:29

save work

c9da117

save work

7ad7a1a

wcy-fdu marked this pull request as draft May 27, 2024 02:00

wcy-fdu added 4 commits May 27, 2024 11:02

todo, handle ctl

5798276

add some comments

800cc64

refactor

912f913

some rename

7acef06

wcy-fdu requested a review from hzxa21 May 28, 2024 07:14

wcy-fdu marked this pull request as ready for review May 28, 2024 07:15

make clippy happy

b71f9b2

resolve conflict

3685478

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

typo fix

a7dcb71

hzxa21 changed the title ~~feat(object store): devide prefix for azblob object store~~ feat(object store): introduce object prefix for opendal object store May 28, 2024

wcy-fdu added 2 commits May 28, 2024 18:16

fix ut

dcdea1b

fix e2e

c332de6

hzxa21 reviewed May 30, 2024

View reviewed changes

wcy-fdu added 2 commits June 3, 2024 15:38

resolve some comments

c63dc09

resolve some comments

260fac6

wcy-fdu commented Jun 3, 2024

View reviewed changes

wcy-fdu requested a review from hzxa21 June 3, 2024 09:13

hzxa21 reviewed Jun 3, 2024

View reviewed changes

wcy-fdu added 2 commits June 3, 2024 18:59

judge is new cluster before Migrator::up and fix all ctl command

1f837c6

fix ut

ac1b97d

wcy-fdu requested a review from hzxa21 June 4, 2024 01:30

wcy-fdu requested a review from yezizp2012 June 4, 2024 01:30

empty commit for retry

7c5e8c3

hzxa21 reviewed Jun 12, 2024

View reviewed changes

resolve some comment

1f2dfe3

wcy-fdu added the ci/run-backwards-compat-tests Run backwards compatibility tests in your PR. label Jun 12, 2024

wcy-fdu added 2 commits June 12, 2024 17:10

minor

eb628fc

fmrt

0420144

wcy-fdu added 2 commits June 14, 2024 13:14

resolve comment

58581cf

fix ci

87ab951

hzxa21 reviewed Jun 17, 2024

View reviewed changes

src/meta/src/manager/env.rs Outdated Show resolved Hide resolved

src/meta/src/manager/env.rs Outdated Show resolved Hide resolved

src/meta/src/manager/env.rs Outdated Show resolved Hide resolved

wcy-fdu added 2 commits June 17, 2024 14:32

fix

a028c13

style change

ea9befe

wcy-fdu requested a review from hzxa21 June 17, 2024 07:59

hzxa21 approved these changes Jun 17, 2024

View reviewed changes

resolve conflict

6de4852

wcy-fdu enabled auto-merge June 17, 2024 09:06

wcy-fdu added this pull request to the merge queue Jun 17, 2024

Merged via the queue into main with commit 1b8a563 Jun 17, 2024
33 of 34 checks passed

wcy-fdu deleted the wcy/azblob_prefix branch June 17, 2024 09:57

Li0k reviewed Jun 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(object store): introduce object prefix for opendal object store #16542

feat(object store): introduce object prefix for opendal object store #16542

wcy-fdu commented Apr 29, 2024 •

edited

Loading

wcy-fdu commented May 28, 2024

hzxa21 May 30, 2024

wcy-fdu May 30, 2024

wcy-fdu May 30, 2024

wcy-fdu Jun 3, 2024

wcy-fdu Jun 3, 2024

yezizp2012 Jun 3, 2024

hzxa21 Jun 3, 2024

wcy-fdu Jun 3, 2024

hzxa21 Jun 3, 2024

wcy-fdu commented Jun 14, 2024

hzxa21 left a comment

gitguardian bot commented Jun 17, 2024

Li0k Jun 18, 2024

feat(object store): introduce object prefix for opendal object store #16542

feat(object store): introduce object prefix for opendal object store #16542

Conversation

wcy-fdu commented Apr 29, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

wcy-fdu commented May 28, 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

Choose a reason for hiding this comment

wcy-fdu commented Jun 14, 2024

hzxa21 left a comment

Choose a reason for hiding this comment

gitguardian bot commented Jun 17, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Choose a reason for hiding this comment

wcy-fdu commented Apr 29, 2024 •

edited

Loading