laskoviymishka commented on code in PR #3179:
URL: https://github.com/apache/iceberg-rust/pull/3179#discussion_r4066580002
##########
crates/iceberg/src/io/storage/config/s3.rs:
##########
@@ -64,6 +64,12 @@ pub const S3_ALLOW_ANONYMOUS: &str = "s3.allow-anonymous";
pub const S3_DISABLE_EC2_METADATA: &str = "s3.disable-ec2-metadata";
/// Option to skip loading configuration from config file and the env.
pub const S3_DISABLE_CONFIG_LOAD: &str = "s3.disable-config-load";
+/// Size in bytes of each part of a multipart upload. Must be between 5 MiB
and 5 GiB.
+/// Defaults to 32 MiB, matching Java `S3FileIOProperties.MULTIPART_SIZE`.
Review Comment:
`MULTIPART_SIZE` is the property key in Java; the 32 MiB value is
`MULTIPART_SIZE_DEFAULT`. The private const in `s3.rs` already cites
`MULTIPART_SIZE_DEFAULT` correctly, so this public one should match it.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -395,6 +401,27 @@ impl OpenDalStorage {
Ok((operator, relative_path))
}
+ /// Bounds the S3 request size. Without a chunk size OpenDAL turns each
caller
+ /// buffer into one request, and `ParquetWriter` hands over a whole row
group
+ /// at a time. Other backends keep OpenDAL's defaults.
+ #[allow(unreachable_patterns)]
+ fn write_options(&self) -> WriteOptions {
+ match self {
+ #[cfg(feature = "opendal-s3")]
+ OpenDalStorage::S3 {
+ multipart_part_size,
+ ..
+ } => WriteOptions {
+ // A validated part size is at most 5 GiB, which still exceeds
+ // `usize` on a 32-bit target. Clamp there instead of panicking
+ // on a configured value.
+ chunk:
Some(usize::try_from(*multipart_part_size).unwrap_or(usize::MAX)),
Review Comment:
Small thing, but the comment stops just short of the actual behavior: on a
32-bit target `usize::MAX` is ~4 GiB, so a configured 5 GiB part size silently
applies as ~4 GiB even though the validator accepted it. Worth a word in the
comment so the effective ceiling isn't a surprise. Not blocking.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -543,7 +570,7 @@ impl Storage for OpenDalStorage {
async fn write(&self, path: &str, bs: Bytes) -> Result<()> {
let (op, relative_path) = self.create_operator(&path)?;
- op.write(relative_path, bs)
+ op.write_options(relative_path, bs, self.write_options())
Review Comment:
Last round I wasn't sure this path chunked at all and floated dropping it —
`test_file_io_s3_write_splits_buffer_into_bounded_parts` settles it, it does
split. And `test_file_io_s3_write_below_part_size_stays_single_request` closes
the other half of my worry: small metadata writes stay a single PUT, so
commit-heavy workloads don't eat a 3x round-trip. Good answer.
One thing I want to be sure of: these assertions all lean on MinIO, so they
only mean something if they actually run in CI rather than just locally. Can we
confirm the S3 integration job runs on this PR and is green? Once that's
confirmed I'm satisfied on this path.
##########
crates/storage/opendal/tests/file_io_s3_test.rs:
##########
@@ -26,33 +26,68 @@ mod tests {
use bytes::Bytes;
use futures::StreamExt;
use iceberg::io::{
- FileIO, FileIOBuilder, S3_ACCESS_KEY_ID, S3_ENDPOINT,
S3_PATH_STYLE_ACCESS, S3_REGION,
- S3_SECRET_ACCESS_KEY,
+ FileIO, FileIOBuilder, S3_ACCESS_KEY_ID, S3_ENDPOINT,
S3_MULTIPART_PART_SIZE_BYTES,
+ S3_PATH_STYLE_ACCESS, S3_REGION, S3_SECRET_ACCESS_KEY,
};
use iceberg_storage_opendal::{
AwsCredential, CustomAwsCredentialLoader, OpenDalStorageFactory,
ProvideCredential,
};
use iceberg_test_utils::{get_minio_endpoint,
normalize_test_name_with_parts, set_up};
+ use opendal::Configurator;
use reqsign_core::Context;
async fn get_file_io() -> FileIO {
+ get_file_io_with_props(vec![]).await
+ }
+
+ async fn get_file_io_with_props(extra: Vec<(&'static str, String)>) ->
FileIO {
set_up();
let minio_endpoint = get_minio_endpoint();
- FileIOBuilder::new(Arc::new(OpenDalStorageFactory::S3 {
- customized_credential_load: None,
- }))
- .with_props(vec![
+ let mut props = vec![
(S3_ENDPOINT, minio_endpoint),
(S3_ACCESS_KEY_ID, "admin".to_string()),
(S3_SECRET_ACCESS_KEY, "password".to_string()),
(S3_REGION, "us-east-1".to_string()),
(S3_PATH_STYLE_ACCESS, "true".to_string()),
- ])
+ ];
+ props.extend(extra);
+
+ FileIOBuilder::new(Arc::new(OpenDalStorageFactory::S3 {
+ customized_credential_load: None,
+ }))
+ .with_props(props)
.build()
}
+ /// Number of upload parts S3 recorded for `key`. A multipart upload
reports
+ /// a `<md5>-<part-count>` ETag; a single-request upload reports a bare
MD5.
+ ///
+ /// The suffix is a MinIO and plain-S3 behavior. A server-side encrypted
+ /// object can report an ETag without it, so this helper holds only for the
+ /// MinIO fixture these tests run against.
+ async fn upload_part_count(key: &str) -> usize {
+ let mut config = opendal::services::S3Config::default();
+ config.endpoint = Some(get_minio_endpoint());
+ config.access_key_id = Some("admin".to_string());
Review Comment:
This helper re-hardcodes the creds and bucket instead of going through
`get_minio_endpoint()` and the shared props. If the fixture ever changes, the
`stat` here fails with a 403 and `.expect("MinIO reports an ETag")` reports it
as a missing ETag — misleading. Could we pull these from the same place the
other tests do?
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -233,6 +235,9 @@ pub enum OpenDalStorage {
S3 {
/// S3 configuration.
config: Arc<S3Config>,
+ /// Bytes carried by one multipart upload request.
+ #[serde(default = "default_multipart_part_size")]
+ multipart_part_size: u64,
Review Comment:
This is the pub-vs-`pub(crate)` call I flagged last round, still open. Since
users set the size through the `s3.multipart.part-size-bytes` property either
way, I don't think the field needs to be publicly constructible — `pub(crate)`
would keep the surface smaller.
If we do want it public, two things worth doing while we're here: mark the
`S3 { }` variant `#[non_exhaustive]` so the next S3 option isn't another `fix!`
bump (CTTY raised the same threading concern), and expose a `pub const` for the
32 MiB default so a downstream constructor isn't hardcoding `33_554_432`.
Either direction is fine — I'd just like a deliberate call rather than it
defaulting to public. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
