apache
feat: initial `Operator::from_uri` implementation
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.
hi @Xuanwo, thank you very much for your review. I answered all the comments and I'm ready for another round.
Hi, @jorgehermo9, I'm currently addressing some urgent existing issues in the current OpenDAL release. We will be publishing one or more patch versions for OpenDAL 0.51.x, and this PR (which introduces breaking changes) will be merged afterward. I will return to review this PR once I have resolved everything.
Don't worry, take your time! Just pinging to notify, but no hurries! Thank you very much for your hard work
Hi @Xuanwo, just a little remind in case this got forgotten. I have more time right now to keep working on this
Hi @asukaminato0721 are you interested on this PR? I was waiting for @Xuanwo next review, I'm open to keep working on this anytime
https://github.com/apache/opendal/pull/5616 so libsql can be removed. https://github.com/apache/opendal/issues/2199#issuecomment-2152251294
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!
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
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 opendal, opendal-core, opendal-services-xxx, and opendal-layers-xxx as we discussed in https://github.com/apache/opendal/discussions/5206. In this phase, we’ll figure out how to split opendal and design a registry mechanism that is easy to use and works well for opendal after the split. Based on that, we can design a from_uri API that can reuse our existing framework. At that point, adding the from_uri API should be simple and won’t introduce any breaking changes.
I led you down the wrong path because I used to think it was fine to add from_uri first and then perform the splits. However, during the review of this PR, I finally realized that I had misunderstood these two processes. In fact, we can't have a proper from_uri unless we know how the opendal split and opendal registry will look.
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.
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.
Thank you very much for your understanding!
. 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.
Yes, let's keep this open as a draft.