Skip to content

feat(storage/opendal): route oss:// URLs through the S3 backend#2332

Open
plusplusjiajia wants to merge 1 commit intoapache:mainfrom
plusplusjiajia:feat/oss-routes-through-s3
Open

feat(storage/opendal): route oss:// URLs through the S3 backend#2332
plusplusjiajia wants to merge 1 commit intoapache:mainfrom
plusplusjiajia:feat/oss-routes-through-s3

Conversation

@plusplusjiajia
Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

What changes are included in this PR?

Aliyun OSS exposes an S3-compatible API, and the other two Iceberg implementations already read oss:// tables through their S3 code path with canonical s3.* properties:

iceberg-rust routes oss:// to opendal's native Oss service instead, which reads a separate oss.* namespace and can't be driven by the same credentials. Align with the rest of the ecosystem.

Copy link
Copy Markdown
Collaborator

@CTTY CTTY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @plusplusjiajia thanks for the contribution! Logic wise this makes sense, but I'm concerned that this will confuse users that OSS storage has two modes. Do you think we should just keep one?

#[cfg(feature = "opendal-oss")]
// OSS is S3-API-compatible; route through S3 so `s3.*` props
// work for OSS-backed tables (mirrors pyiceberg/Java S3FileIO).
#[cfg(feature = "opendal-s3")]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg(feature = "opendal-s3")]
#[cfg(all(feature = "opendal-oss", feature = "opendal-s3"))]

Only having s3 feature flag will introduce oss related code to non-oss users

}
// Fallback: builds without `opendal-s3` but with `opendal-oss`
// still use the native OSS service (which consumes `oss.*` keys).
#[cfg(all(feature = "opendal-oss", not(feature = "opendal-s3")))]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with OSS storage so not sure about it: Is the fallback actually necessary? I'm thinking if using OSS exactly like S3 is the common case, maybe we can just use OpenDalS3Storage directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants