feat: initial `Operator::from_uri` implementation #5482

jorgehermo9 · 2024-12-30T19:41:37Z

Relates to #5445

Left some doubts as // TODO in the source. I have little experience contributing to this repo so I'm sorry if there are a lot of doubts about this. Just want to be sure all the changes of this PR aligns with the current codebase. Please take a look to all the TODOs I left when reviewing.

I would like to add more tests, but I don't know in which place those should be placed. The core/tests folder seems like a good place, but I don't find any place suitable, as placing those in core/tests/behaviour seems weird to me. But as this implies various components, maybe we can have a core/tests/integration? Although I would like to write some unit tests at core/src/types/builder.rs, core/src/types/operator/builder.rs and core/src/types/operator/registry.rs, but didn't any existing unit tests there.

In this PR I implemented a single Configurator::from_uri method, which will serve as default and takes only the query parameters as options. Services which need a more specific configuration such as s3 or azblob can be implemented in follow-up PRs.

I also have pending documentating all the newly added public API, but will do that after an initial review round.

Thank you very much.

jorgehermo9 · 2024-12-30T19:52:50Z

core/src/types/operator/builder.rs

+    ///
+    /// Ok(())
+    /// ```
+    pub fn from_uri(


should this method be called via_uri?

The from_iter method returns an OperatorBuilder instead of Operator

opendal/core/src/types/operator/builder.rs

Line 147 in 6eed396

) -> Result<OperatorBuilder<impl Access>> {

and the via_iter method returns an Operator (as this from_uri method does)

opendal/core/src/types/operator/builder.rs

Line 185 in 6eed396

) -> Result<Operator> {

Should we include two new methods? via_uri and from_uri?

from_uri is good enough, all via_xxx API may be removed in the future.

jorgehermo9 · 2024-12-30T19:53:44Z

core/src/types/operator/registry.rs

+    }
+}
+
+fn operator_factory_from_configurator<C: Configurator>() -> OperatorFactory {


included this private function to reduce boilerplate of operator factory generation. Any suggestion about it or is it ok to have it here?

I prefer to remove operator_factory_from_configurator.

jorgehermo9 · 2024-12-30T19:55:18Z

core/src/types/operator/registry.rs

@@ -0,0 +1,226 @@
+use std::cell::LazyCell;


I added this file inside the crate::types::operator::registry module. Is it ok? I thought about adding a crate::types::operator_registry, but it seemed better this way.

jorgehermo9 · 2024-12-30T19:56:25Z

core/src/types/operator/registry.rs

+
+// TODO: thread local or use LazyLock instead? this way the access is lock-free
+// TODO: should we expose the `GLOBAL_OPERATOR_REGISTRY` as public API at `crate::types::operator::GLOBAL_OPERATOR_REGISTRY`?
+thread_local! {


What is the preferred way of having a global static variable such as this?

I prefer to have it thread_local so there is not need for a LazyLock, we can use LazyCell instead (LazyCell is lock-free but LazyLock is not, it synchronizes access through threads)

jorgehermo9 · 2024-12-30T20:08:58Z

core/src/types/operator/registry.rs

+// TODO: thread local or use LazyLock instead? this way the access is lock-free
+// TODO: should we expose the `GLOBAL_OPERATOR_REGISTRY` as public API at `crate::types::operator::GLOBAL_OPERATOR_REGISTRY`?
+thread_local! {
+    pub static GLOBAL_OPERATOR_REGISTRY: LazyCell<OperatorRegistry> = LazyCell::new(|| OperatorRegistry::with_enabled_services());


MSRV check fails due to the usage of LazyCell. Should we update the MSRV or use another thing instead?

I see this TODO about the once_cell crate usage:

opendal/core/Cargo.toml

Line 248 in 52c96bb

# TODO: remove once_cell when lazy_lock is stable: https://doc.rust-lang.org/std/cell/struct.LazyCell.html

If the replacement is planned, I think it would be better to use LazyCell than once_cell::Lazy in new code like this one, to not increase technical debt.

Xuanwo · 2024-12-31T07:24:23Z

core/Cargo.toml

@@ -255,6 +255,9 @@ reqwest = { version = "0.12.2", features = [
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 tokio = { version = "1.27", features = ["sync", "io-util"] }
+# TODO: I added this dependency in order to not re-implement the Url::query_pairs function.
+# Is it okay to include it? (we are using that crate in the ofs binary https://github.com/apache/opendal/blob/main/bin/ofs/Cargo.toml#L45
+url = "2.5.4"


Please use http::uri instead. During the development of core, there is no need to reference other download stream projects. Simply make the best decision based on the current core code.

Xuanwo · 2024-12-31T07:24:58Z

core/src/types/builder.rs

@@ -137,6 +137,51 @@ pub trait Configurator: Serialize + DeserializeOwned + Debug + 'static {
        })
    }

+    // TODO: should we split `from_uri` into two functions? `from_uri` and `from_uri_opts`?


Doesn't worth it.

Xuanwo · 2024-12-31T07:30:20Z

core/src/types/operator/registry.rs

+// Register only services in `Scheme::enabled()`
+
+pub struct OperatorRegistry {
+    // TODO: add Arc<Mutex<...>> to make it cheap to clone + thread safe? or is it not needed?


It makes sense to me; we rarely change it or keep the lock engaged for long.

Xuanwo · 2024-12-31T07:30:40Z

core/src/types/operator/registry.rs

+}
+
+impl OperatorRegistry {
+    pub fn new() -> Self {


Let's initialize the registry here directly.

Xuanwo · 2024-12-31T07:33:21Z

core/src/types/operator/registry.rs

+    }
+}
+
+fn operator_factory_from_configurator<C: Configurator>() -> OperatorFactory {


I prefer to remove operator_factory_from_configurator.

Xuanwo · 2024-12-31T07:38:47Z

core/src/types/operator/registry.rs

+        // ```
+        // and the apply_for_all_services macro would gate every statement behind the corresponding feature gate
+        // This seems to not be the place where we should have a "list of enabled services".
+        #[cfg(feature = "services-aliyun-drive")]


I believe every service should have its own registration functions. There are two reasons:

The service may have different schemes for registration. For example, S3 registers as s3, s3a, minio, r2, and so on, while GCS registers as gcs, gs, and so forth.

Provide a register that simplifies integration with OpenDAL for external users.

jorgehermo9 added 7 commits December 30, 2024 13:27

feat: initial OperatorRegistry implementation

f2267e6

feat: continue with operator registry implementation

f581316

feat: register enabled services & set global registry

b67be38

feat: glue together Operator and global OperatorRegistry

b8c4fb1

feat: glue together Operator and global OperatorRegistry

a50c058

feat: implement Configurator::from_uri

539e6ea

test: add small doctest to Operator::from_uri

3aba98b

jorgehermo9 requested a review from Xuanwo as a code owner December 30, 2024 19:41

github-actions bot added the releases-note/feat label Dec 30, 2024

jorgehermo9 added 2 commits December 30, 2024 20:42

chore: add comment

5979898

chore: remove changes

6eed396

jorgehermo9 commented Dec 30, 2024

View reviewed changes

chore: remove changes

0e582ee

jorgehermo9 commented Dec 30, 2024

View reviewed changes

jorgehermo9 added 2 commits December 30, 2024 21:03

chore: add license header

9d730c0

chore: fix typo

976e60c

jorgehermo9 commented Dec 30, 2024

View reviewed changes

jorgehermo9 added 2 commits December 30, 2024 21:10

fix: clippy lint

a4478a9

retrigger ci

a4e3866

Xuanwo reviewed Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial `Operator::from_uri` implementation #5482

feat: initial `Operator::from_uri` implementation #5482

jorgehermo9 commented Dec 30, 2024 •

edited

Loading

jorgehermo9 Dec 30, 2024

Xuanwo Dec 31, 2024

jorgehermo9 Dec 30, 2024

Xuanwo Dec 31, 2024

jorgehermo9 Dec 30, 2024

jorgehermo9 Dec 30, 2024

jorgehermo9 Dec 30, 2024

Xuanwo Dec 31, 2024

Xuanwo Dec 31, 2024

Xuanwo Dec 31, 2024

Xuanwo Dec 31, 2024

Xuanwo Dec 31, 2024

Xuanwo Dec 31, 2024

feat: initial Operator::from_uri implementation #5482

Are you sure you want to change the base?

feat: initial Operator::from_uri implementation #5482

Conversation

jorgehermo9 commented Dec 30, 2024 • edited Loading

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

feat: initial `Operator::from_uri` implementation #5482

feat: initial `Operator::from_uri` implementation #5482

jorgehermo9 commented Dec 30, 2024 •

edited

Loading