refactor: Use opendal to replace object_store #978
Conversation
Signed-off-by: Xuanwo <[email protected]>
Xuanwo
force-pushed
the
storage-refactor
branch
from
7d04759 to
cfad6c8
Compare
Signed-off-by: Xuanwo <[email protected]>
…nto storage-refactor
Signed-off-by: Xuanwo <[email protected]>
|
Some tests are failing. I'm working on it. |
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
|
@Xuanwo did you look at the latest CI by a chance? |
Signed-off-by: Xuanwo <[email protected]>
|
Hi, the only thing left is |
|
@Xuanwo we dont have any S3 bucket used for testing, in these specific tests it seems the old implementation would not actually connect to S3 (because the test is testing for failure). |
Thank you for the information, I will take an another look. |
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
|
Hi, @jondot sorry for the long waiting. All tests should pass now. Please take another look, thank you! |
| let fs = Fs::default().root("/"); | ||
| Box::new(OpendalAdapter::new( | ||
| Operator::new(fs) | ||
| .expect("fs service should build with success") |
There was a problem hiding this comment.
Can we handle this without panic?
Would it make more sense to return a Result instead?
There was a problem hiding this comment.
Hi, I'm willing to return an error if changing the signature is allowed. This PR tries it's best to not change the exposed public API.
There was a problem hiding this comment.
I think it is better than I doing expect.
What do you think? it should't affect the users, right? Only framework changes
There was a problem hiding this comment.
Fxied, please review again. Sorry for the late 💌
| Box::new(ObjectStoreAdapter::new(Box::new(InMemory::new()))) | ||
| Box::new(OpendalAdapter::new( | ||
| Operator::new(Memory::default()) | ||
| .expect("memory service must build with success") |
|
Thanks @Xuanwo. |
Hi, this is what I mentioned in #978 (comment). In fact, the builders for |
|
got it. so can you please revert this change? sorry for that after the ci pass we will merge it thanks a lot |
Don't worry, I get it. Let me fix this. |
Xuanwo
force-pushed
the
storage-refactor
branch
from
d627c9c to
86e7b31
Compare
|
Hi, @kaplanelad I have force pushed the changes, please review again. Thank you for the review 💌 |
Close #948
This PR will use opendal to replace object_store.
The most significant change is the addition of the
GetResponsestruct. After this PR,GetResponsewill no longer expose any external types; instead, it will provide its own public API that users can rely on.cc @jondot to take a review, thank you!
There are many things we can consider, such as enabling logging, tracing, and metrics layers to make our storage driver more production-ready. Perhaps we can discuss this later in a separate issue.