Skip to content

fix(io): bound S3 write request size with a configurable part size - #3179

Open
YuangGao wants to merge 1 commit into
apache:mainfrom
YuangGao:fix/s3-multipart-part-size
Open

fix(io): bound S3 write request size with a configurable part size#3179
YuangGao wants to merge 1 commit into
apache:mainfrom
YuangGao:fix/s3-multipart-part-size

Conversation

@YuangGao

@YuangGao YuangGao commented Sep 9, 2026

Copy link
Copy Markdown

Which issue does this PR close?

What changes are included in this PR?

OpenDAL turns one caller buffer into one request, and ParquetWriter hands over a whole row group per write call. A 128 MiB row group therefore became a single UploadPart racing the hard-coded 10s IO timeout, and since every retry re-sends the same oversized request, the write fails deterministically rather than flakily.

Bound the request size so it follows configuration instead of the caller's buffer:

  • Add s3.multipart.part-size-bytes, default 32 MiB (matching Java S3FileIOProperties.MULTIPART_SIZE_DEFAULT), rejected below the 5 MiB S3 minimum for a non-final part.
  • OpenDalStorage::S3 applies it through write_options / writer_options. Other backends keep OpenDAL's defaults.

Are these changes tested?

  • Unit tests for property parsing: default, override, 5 MiB boundary, non-numeric input.
  • Integration test against MinIO asserting one FileWrite::write is split into the configured number of upload parts, counted from the multipart ETag suffix.
  • Verified with mc admin trace: a 160 MiB Parquet write previously issued one request, now issues 5 x 32 MiB parts plus a remainder.

AI Disclosure

  • AI-assisted implementation.

@anoopj anoopj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CI check-public-api is failing. This PR changes a couple of public API surfaces that aren't reflected in the checked-in public-api.txt files.

Please run make generate-public-api and commit both updated files.

config: Arc<S3Config>,
/// Bytes carried by one multipart upload request.
#[serde(default = "default_multipart_part_size")]
multipart_part_size: usize,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This field is serialized and is round-tripped. A usize-typed serialized value ties the wire format to host word size. This might cause issues if we have heterogeneous setup (granted, they are rare). Consider storing u64 and converting as usize somewhere in the edge.

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