-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Use opendal to replace object_store #978
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Xuanwo <[email protected]>
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Xuanwo! see my question about the panic
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we handle this without panic?
Would it make more sense to return a Result instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better than I don't expect.
What do you think? it should't affect the users, right? Only framework changes
Box::new(ObjectStoreAdapter::new(Box::new(InMemory::new()))) | ||
Box::new(OpendalAdapter::new( | ||
Operator::new(Memory::default()) | ||
.expect("memory service must build with success") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question
Close #948
This PR will use opendal to replace object_store.
The most significant change is the addition of the
GetResponse
struct. After this PR,GetResponse
will 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.