feat: initial Operator::from_uri implementation#5482
feat: initial Operator::from_uri implementation#5482jorgehermo9 wants to merge 38 commits intoapache:mainfrom
Operator::from_uri implementation#5482Conversation
github-actions
bot
added
the
releases-note/feat
core/src/types/operator/registry.rs
Outdated
| @@ -0,0 +1,226 @@ | |||
| use std::cell::LazyCell; | |||
There was a problem hiding this comment.
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.
core/src/types/operator/registry.rs
Outdated
|
|
||
| // 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! { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
just curious, why do we need static var here?
There was a problem hiding this comment.
In order to have a global initialized registry and to not have to initialize it every time whenever we want to parse a uri
opendal/core/src/types/operator/builder.rs
Line 118 in 088d7d0
core/src/types/operator/registry.rs
Outdated
| // 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()); |
There was a problem hiding this comment.
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:
Line 248 in 52c96bb
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.
|
so many "todo"... keep the code change small, clippy happy, test pass, and the code style unified (you can use llm for this kind of idea) then you can answer many todos by yourself. :) |
|
Thank you very much for your review @asukaminato0721! I usually leave the TODOs in code as comments to the reviewer, will address everything asap! |
| fn from_uri(uri: &Uri, options: impl IntoIterator<Item = (String, String)>) -> Result<Self> { | ||
| let query_pairs = uri.query().map(query_pairs).unwrap_or_default(); | ||
|
|
||
| let merged_options = query_pairs.into_iter().chain(options); |
There was a problem hiding this comment.
Unsure about
let merged_options = query_pairs.into_iter().chain(options);
or
let merged_options = options.chain(query_pairs.into_iter());
Which one should take precedence? query_pairs should overwrite the options, or should the options overwrite the query_pairs? I'm thinking on the case that query_pairs and options contain the same key
Right now, I think that the behaviour with query_pairs.into_iter().chain(options) is that options takes precedence and them overwrite whatever goes in query_pairs if a key is present in both
| pub use registry::OperatorFactory; | ||
| pub use registry::OperatorRegistry; | ||
| pub use registry::GLOBAL_OPERATOR_REGISTRY; |
There was a problem hiding this comment.
should we expose these 3 as public api @Xuanwo? or should we pub(crate) them instead?
Or should we only expose OperatorFactory and OperatorRegistry but not GLOBAL_OPERATOR_REGISTRY?
|
Hi @Xuanwo, I worked a bit more on this and I think it is ready for another review round. Once the implementation looks good to you, I will work on tests and documentation, is this okay? @asukaminato0721 I think I addressed most of your comments |
| } | ||
|
|
||
| /// TODO: document this. | ||
| fn from_uri(uri: &Uri, options: impl IntoIterator<Item = (String, String)>) -> Result<Self> { |
There was a problem hiding this comment.
Is it okay to accept &Uri here? This way, we don't have to parse the uri again (it is only done here right now)
Or should we change it to &str and parse the uri twice?
|
Sorry, @jorgehermo9, for keeping you waiting so long. I plan to start working on this soon. The main issue preventing us from moving forward smoothly is that we haven’t yet decided how to split the crates and design our registry. I think we can break this feature into two parts: one for splitting opendal, which will divide opendal into I led you down the wrong path because I used to think it was fine to add I think we can keep this PR as a draft, and I'll invite you to join the discussion about the upcoming OpenDAL split RFC. I believe your experience in implementing this PR could be really valuable. |
|
No problem at all @Xuanwo, I completely understand that. Thank you very much for your review and thoughts, and I'll be happy to discuss it on the RFC. I'm leaving the PR as a draft for future reference until the RFC is done, but feel free to close it if you want. |
Thank you very much for your understanding!
Yes, let's keep this open as a draft. |
4 participants
Relates to #5445
Left some doubts as
// TODOin 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 theTODOsI left when reviewing.I would like to add more tests, but I don't know in which place those should be placed. The
core/testsfolder seems like a good place, but I don't find any place suitable, as placing those incore/tests/behaviourseems weird to me. But as this implies various components, maybe we can have acore/tests/integration? Although I would like to write some unit tests atcore/src/types/builder.rs,core/src/types/operator/builder.rsandcore/src/types/operator/registry.rs, but didn't any existing unit tests there.In this PR I implemented a single
Configurator::from_urimethod, 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.