laskoviymishka commented on code in PR #3179:
URL: https://github.com/apache/iceberg-rust/pull/3179#discussion_r4119697065
##########
crates/iceberg/src/io/storage/config/s3.rs:
##########
@@ -64,6 +64,15 @@ 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_DEFAULT`.
+///
+/// Each part travels as one request, so the part size bounds how much data
has to
+/// transfer within the per-operation IO timeout. Lower it on a slow
connection.
+///
+/// Java stores this property in an `int` and its `S3FileIOProperties` javadoc
asks for
+/// a part size below 2 GB, so stay under that for a value both clients accept.
+pub const S3_MULTIPART_PART_SIZE_BYTES: &str = "s3.multipart.part-size-bytes";
Review Comment:
Every other const in this file (`S3_ENDPOINT`, `S3_DISABLE_CONFIG_LOAD`,
...) has a matching `S3Config` field and a `TryFrom<&StorageConfig>` arm; this
one has neither. It's not a runtime break today since the opendal factory path
is what consumes it, but it's a gap in this file's own invariant — I'd either
wire in the field + validation arm or drop a one-line comment that the const is
intentionally consumed only by the opendal storage crate.
##########
crates/storage/opendal/src/s3.rs:
##########
@@ -36,6 +36,49 @@ use url::Url;
use crate::utils::{from_opendal_error, is_truthy};
+/// S3 rejects a non-final part smaller than this.
+const MULTIPART_PART_SIZE_MIN: u64 = 5 * 1024 * 1024;
+
+/// S3 rejects a part larger than this.
+const MULTIPART_PART_SIZE_MAX: u64 = 5 * 1024 * 1024 * 1024;
Review Comment:
I'd pull `MULTIPART_PART_SIZE_MAX` down to `i32::MAX` — at 5 GiB it
contradicts the const's own doc, which tells users to stay under 2 GB so a Java
client accepts the same property. A value in [~2 GiB, 5 GiB) passes here but
hard-fails Java-side FileIO construction, since
`S3FileIOProperties.multiPartSize` is an `int` and `PropertyUtil.propertyAsInt`
throws above `Integer.MAX_VALUE`. If keeping 5 GiB is deliberate for Rust-only,
then the doc should say that outright rather than claim a 2 GB ceiling we don't
actually enforce.
##########
crates/iceberg/src/io/storage/config/s3.rs:
##########
@@ -64,6 +64,15 @@ 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_DEFAULT`.
+///
+/// Each part travels as one request, so the part size bounds how much data
has to
+/// transfer within the per-operation IO timeout. Lower it on a slow
connection.
Review Comment:
The doc tells people to lower this on a slow link, but it doesn't say the 5
MiB floor is a hard bottom and `io_timeout` stays fixed at 10s — so a link that
can't push 5 MiB in 10s hits the same failure with no lever left. A sentence
noting that, plus a pointer to #2977 for the follow-up (configurable /
size-scaled timeout), keeps the shipped doc honest about how far this fix
reaches.
--
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]
