feat(io): add OSS storage implementation#1153
feat(io): add OSS storage implementation#1153Xuanwo merged 1 commit intoapache:mainfrom divinerapier:main
Conversation
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @divinerapier for this pr. But it seems that supporting oss scheme in s3 would be enough? Also there is no tests for it.
crates/iceberg/src/io/storage.rs
Outdated
| Oss { | ||
| /// oss storage could have `oss://`. | ||
| /// Storing the scheme string here to return the correct path. | ||
| scheme_str: String, |
There was a problem hiding this comment.
Do we really need this? We need it for s3 to be compatible with both s3 and s3a, is this also the case for oss?
There was a problem hiding this comment.
I mean why we need to keep the schema_str field for oss?
There was a problem hiding this comment.
You're right, I will fix it .
There was a problem hiding this comment.
#[cfg(feature = "storage-oss")]
Oss {
/// uses the same client for one FileIO Storage.
///
/// TODO: allow users to configure this client.
client: reqwest::Client,
config: Arc<OssConfig>,
},Fixed, PTAL @liurenjie1024
|
@Xuanwo PTAL 😆 I am in the process of implementing support for Aliyun OSS storage backend using OpenDAL, can you provide me with some advice on how to test this? Thanks. |
There are no good OSS-compatible services like MinIO for S3 that suit our testing needs. OpenDAL tests OSS using a user-sponsored, dedicated testing bucket. Perhaps we should consider taking a similar approach. |
Thank you (@Xuanwo ) for raising this critical issue about OSS-compatible testing infrastructure. I fully agree that a real OSS service would be ideal for ensuring authentic testing behavior. I previously contacted Aliyun (OSS) pre-sales support to inquire about dedicated testing environments, but unfortunately, they currently do not offer such programs for open-source projects. Given OpenDAL's successful model of using community-sponsored resources, I want to explore two potential paths:
|
|
I don't think it's a blocker. @liurenjie1024, do you think it's a good idea to implement it first but leave it out of |
|
and run it by: cargo run --package iceberg-examples --example oss-backend --features storage-oss |
Sounds reasonable to me, but we need to add documentation to explain why we don't include it int |
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @divinerapier for this pr, LGTM!
|
Hi, @divinerapier would you like to merge with main to fix the CI? |
Signed-off-by: divinerapier <sihao.fang@outlook.com>
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you @divinerapier for working on this, really nice!
Would you like to submit a seperate issue for this? |
|
An issue submitted #1188 |
- [x] Support AliyunOSS backend by OpenDAL - [x] Update doc explains why feature `storage-oss` is not included in `storage-all`. - [x] Fix typo <!-- Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates. --> <!-- Specify what test covers (unit test, integration test, etc.). If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> The support for AliyunOSS is based on OpenDAL, a production-verified project. Therefore, no additional testing has been added. Add a new example. Signed-off-by: divinerapier <sihao.fang@outlook.com>
- [x] Support AliyunOSS backend by OpenDAL - [x] Update doc explains why feature `storage-oss` is not included in `storage-all`. - [x] Fix typo <!-- Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates. --> <!-- Specify what test covers (unit test, integration test, etc.). If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> The support for AliyunOSS is based on OpenDAL, a production-verified project. Therefore, no additional testing has been added. Add a new example. Signed-off-by: divinerapier <sihao.fang@outlook.com> Co-authored-by: divinerapier <sihao.fang@outlook.com>

What changes are included in this PR?
storage-ossis not included instorage-all.Are these changes tested?
The support for AliyunOSS is based on OpenDAL, a production-verified project. Therefore, no additional testing has been added.
Add a new example.