AWS: Add chunked encoding configuration for S3 requests by plusplusjiajia · Pull Request #15242 · apache/iceberg

plusplusjiajia · 2026-02-06T03:39:12Z

https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/services/s3/S3Configuration.html#chunkedEncodingEnabled()

steveloughran · 2026-02-08T12:34:29Z

what about a test which actually enables it, such as TestS3MultipartUpload?
documentation

steveloughran · 2026-02-09T17:05:25Z

Actually, I've just noticed that org.apache.iceberg.aws.s3.signer.S3V4RestSignerClient doesn't support chunked encoding.

Until that's in, RestCatalog access via S3FileIO isn't going to work.

It'd be OK for direct S3 IO though

plusplusjiajia · 2026-02-10T09:33:40Z

what about a test which actually enables it, such as TestS3MultipartUpload?

documentation

Thanks for you advises, I've added testMultipartUploadWithChunkedEncoding, lease help me take a look, thanks!

steveloughran

commented

test looks good -I always test against real S3 stores so it's nice to see a unit test pushing up a few MB without problems.

steveloughran · 2026-02-10T11:58:29Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java

+   */
+  public static final String CHUNKED_ENCODING_ENABLED = "s3.chunked-encoding-enabled";
+
+  public static final boolean CHUNKED_ENCODING_ENABLED_DEFAULT = true;


this must be false by default because org.apache.iceberg.aws.s3.signer.S3V4RestSignerClient will not sign chunked encoding operations.

this must be false by default because org.apache.iceberg.aws.s3.signer.S3V4RestSignerClient will not sign chunked encoding operations.

Thanks! done.

steveloughran

thanks. My concerns are all addressed.

Afraid you now need to await a review by an iceberg committer

plusplusjiajia · 2026-02-25T09:02:07Z

@amogh-jahagirdar could you help to review this pr?

JingsongLi

Looks good to me!

JingsongLi · 2026-02-27T05:45:23Z

@singhpk234 @huaxingao Can you help us take a look? Just added a configuration.

singhpk234 · 2026-02-27T07:51:34Z

open-api/rest-catalog-open-api.py

     - `s3.session-token`: if present, this value should be used for as the session token
     - `s3.remote-signing-enabled`: if `true` remote signing should be performed as described in the `s3-signer-open-api.yaml` specification
     - `s3.cross-region-access-enabled`: if `true`, S3 Cross-Region bucket access is enabled
+     - `s3.chunked-encoding-enabled`: if `true`, chunked encoding is enabled for S3 requests


do we want rest catalogs to returns this ? for now can this a client side catalog property ?

do we want rest catalogs to returns this ? for now can this a client side catalog property ?

Yes, when the server returns storage credentials, the related S3 properties are expected to be applied in S3FileIO directly, the client doesn't need to be aware of or manage S3-specific configurations on its own.

purely client side; all that comes back from the catalog is signed access to a single file and a header to say cache/no-no cache. Now, that's not enough to deal with SSE-C encryption where you'd need to say (encryption: SSE-C, secret="...."), but that's OK because SSE-C is an utter management nightmare and everyone uses SSE-KMS. Which doesn't need any information from the catalog.

purely client side; all that comes back from the catalog is signed access to a single file and a header to say cache/no-no cache. Now, that's not enough to deal with SSE-C encryption where you'd need to say (encryption: SSE-C, secret="...."), but that's OK because SSE-C is an utter management nightmare and everyone uses SSE-KMS. Which doesn't need any information from the catalog.

makes sense, i will remove it, thanks for the advise!

do we want rest catalogs to returns this ? for now can this a client side catalog property ?

Agreed, keep this as a client-side catalog property. I've removed the change.

singhpk234 · 2026-02-27T07:57:21Z

aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3MultipartUpload.java

+    assertThat(testIo.newInputFile(testObjectUri).getLength())
+        .isEqualTo(parts * (long) S3FileIOProperties.MULTIPART_SIZE_MIN);


i wonder if we can also assert that contents of files are correct ?

steveloughran

commented. I'm making that new test stricter on the basis you've done most of the work and it's good to round off.

FWIW we once hit a prefetch bug in abfs client where under specific race conditions you could get a block from another stream in the same process. This test wouldn't replicate that condition either (2+ GB files, multiple readers, etc), but it'd be a bit stricter.

steveloughran · 2026-02-27T10:28:14Z

aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3MultipartUpload.java

+    verifyInts(testIo, intObjectUri, () -> expectedInt);
+
+    // Verify write and read with bytes
+    byte expectedByte = 42;


nice test, but it this doesn't detect parts committed out of order. What incrementing the expectedByte with each part and validating on the same ranges?

nice test, but it this doesn't detect parts committed out of order. What incrementing the expectedByte with each part and validating on the same ranges?

Good point! I've updated the test.

steveloughran

thanks for the changes. Lost a weekend to a good horror story with a prefetch race condition that shipped. validation of test payloads weren't enough, that payload had to be a 2+GB + CSV file, but still, every safety check helps

https://issues.apache.org/jira/browse/HADOOP-18521

singhpk234

LGTM, thanks @plusplusjiajia

can you please double confirm the integ tests pass ? asking because they are not run as part of CI

…n API spec

plusplusjiajia · 2026-03-04T17:19:15Z

LGTM, thanks @plusplusjiajia

can you please double confirm the integ tests pass ? asking because they are not run as part of CI

Thanks for the reminder, I've verified them manually.

JingsongLi · 2026-03-23T02:21:12Z

Hi @steveloughran , can you take a look again, do you have any other concerns?

steveloughran · 2026-03-23T11:10:17Z

I have no concerns but I'm not a committer. I'll give a finall review and mark if I'm happy with it, but you'll need approval from an iceberg committer.

singhpk234 · 2026-03-25T05:56:04Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java

+  public static final String CHUNKED_ENCODING_ENABLED = "s3.chunked-encoding-enabled";
+
+  public static final boolean CHUNKED_ENCODING_ENABLED_DEFAULT = false;


sorry for the last moment, seems like SDK default is true ? if yes this might be different than what we have been doing so far ? i wonder if we should just pass it to builder if the above propery is not empty and not have defaults ?

@singhpk234 the CatalogSigner doesn't sign chunked encoding
L255

if (signerParams.enableChunkedEncoding()) { throw new UnsupportedOperationException("Chunked encoding not supported"); }

Setting it to false ensures that if a client is using it to sign for uploads (polaris are working on/have done that) then the requests are being signed.
It's likely a failure condition which hasn't yet been encountered

@adutra

I agree for those scenarios we can prompt user to disable it themselves. The way I am approaching is lets say iceberg v 1.10 shipped with chunked encoding true, 1.11 will be shipping ti for false ? this can be regression for no signing use cases ? Hence the recommendation to keep the sdk default but at the same time give ways to override these, wdyt

I agree for those scenarios we can prompt user to disable it themselves. The way I am approaching is lets say iceberg v 1.10 shipped with chunked encoding true, 1.11 will be shipping ti for false ? this can be regression for no signing use cases ? Hence the recommendation to keep the sdk default but at the same time give ways to override these, wdyt

Thanks for the feedback @singhpk234, that's a fair point. Changing the default to false would indeed be a regression for users who don't use the REST catalog signer.

@steveloughran I agree the CatalogSigner incompatibility is a real concern, but I think we can address it without changing the SDK default for everyone. Users who rely on CatalogSigner can explicitly set
s3.chunked-encoding-enabled=false, and we can document this clearly. This way we avoid a silent performance regression for the majority of users while still providing a clear path for signing use cases.

I'll update the PR to keep the default as true.

singhpk234 · 2026-03-25T05:57:55Z

aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3MultipartUpload.java

+  public void testMultipartUploadWithChunkedEncoding(boolean chunkedEncodingEnabled)
+      throws IOException {
+    // Create a new S3FileIO with specified chunked encoding setting
+    S3FileIO testIo = new S3FileIO(() -> s3);


can we wrap with try-with resource , so the testIO is closed ?

can we wrap with try-with resource , so the testIO is closed ?

Thanks for the catch! Updated to use try-with-resources to ensure testIo is properly closed.

…plicating default

singhpk234 · 2026-04-05T20:56:36Z

Thanks for the change @plusplusjiajia ! Thanks for review @JingsongLi @steveloughran !

plusplusjiajia · 2026-04-06T01:01:14Z

Thank you all for taking the time to review this PR!

github-actions bot added the AWS label Feb 6, 2026

plusplusjiajia force-pushed the add-s3-config branch from c8e62ba to 3e6556b Compare February 9, 2026 06:16

github-actions bot added the OPENAPI label Feb 10, 2026

plusplusjiajia force-pushed the add-s3-config branch from 85bdad8 to 1e545f4 Compare February 10, 2026 06:36

steveloughran reviewed Feb 10, 2026

View reviewed changes

steveloughran approved these changes Feb 10, 2026

View reviewed changes

plusplusjiajia force-pushed the add-s3-config branch from 1a71787 to a553eb7 Compare February 14, 2026 14:10

plusplusjiajia force-pushed the add-s3-config branch from a553eb7 to 13aa984 Compare February 25, 2026 08:51

JingsongLi approved these changes Feb 27, 2026

View reviewed changes

singhpk234 reviewed Feb 27, 2026

View reviewed changes

steveloughran reviewed Feb 27, 2026

View reviewed changes

github-actions bot removed the OPENAPI label Feb 27, 2026

plusplusjiajia force-pushed the add-s3-config branch from f0b3852 to 4f81dd0 Compare February 27, 2026 13:22

steveloughran reviewed Feb 27, 2026

View reviewed changes

singhpk234 approved these changes Feb 28, 2026

View reviewed changes

plusplusjiajia force-pushed the add-s3-config branch from 4f81dd0 to d8bcb4f Compare March 4, 2026 02:20

plusplusjiajia added 8 commits March 4, 2026 19:03

AWS: Add chunked encoding configuration for S3 requests

522d98a

add testMultipartUploadWithChunkedEncodingDisabled

5661827

update open api define

15ab7a5

update

345231c

update default value

4e0c68b

update case

ada810a

assert file contents in testMultipartUploadWithChunkedEncoding

9c287e5

Remove s3.chunked-encoding-enabled config entry from REST catalog ope…

496e8f5

…n API spec

plusplusjiajia force-pushed the add-s3-config branch from d8bcb4f to 496e8f5 Compare March 4, 2026 11:04

Use IOUtil.readFully for reliable reads in TestS3MultipartUpload

604fe59

steveloughran approved these changes Mar 23, 2026

View reviewed changes

singhpk234 reviewed Mar 25, 2026

View reviewed changes

plusplusjiajia added 4 commits March 27, 2026 00:31

ensure testIo is properly closed

5948ceb

retrigger CI

85ea1f0

Change chunked encoding default to true to match AWS SDK behavior

0592e17

Fix test to verify explicit disable of chunked encoding instead of du…

8c001e1

…plicating default

singhpk234 approved these changes Apr 5, 2026

View reviewed changes

singhpk234 merged commit ec2de91 into apache:main Apr 5, 2026
36 checks passed

		assertThat(testIo.newInputFile(testObjectUri).getLength())
		.isEqualTo(parts * (long) S3FileIOProperties.MULTIPART_SIZE_MIN);

		public static final String CHUNKED_ENCODING_ENABLED = "s3.chunked-encoding-enabled";

		public static final boolean CHUNKED_ENCODING_ENABLED_DEFAULT = false;

Conversation

plusplusjiajia commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveloughran commented Feb 8, 2026

Uh oh!

steveloughran commented Feb 9, 2026

Uh oh!

plusplusjiajia commented Feb 10, 2026

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

plusplusjiajia commented Feb 25, 2026

Uh oh!

JingsongLi left a comment

Choose a reason for hiding this comment

Uh oh!

JingsongLi commented Feb 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plusplusjiajia Feb 27, 2026 • edited by JingsongLi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

plusplusjiajia commented Mar 4, 2026

Uh oh!

JingsongLi commented Mar 23, 2026

Uh oh!

steveloughran commented Mar 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singhpk234 commented Apr 5, 2026

Uh oh!

plusplusjiajia commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

plusplusjiajia commented Feb 6, 2026 •

edited

Loading

plusplusjiajia Feb 27, 2026 •

edited by JingsongLi

Loading