Propagate storage HTTP client settings to S3FileIO for table operations by iprithv · Pull Request #5004 · apache/polaris

iprithv · 2026-07-08T08:27:25Z

polaris.storage.max-http-connections (and the related read/connect/acquisition/idle/TTL timeouts and expect-continue setting) had no effect on the HTTP clients actually used for table operations.

Users who set polaris.storage.max-http-connections=128 (plus higher polaris.tasks.max-concurrent-tasks) still observed the AWS SDK default of 50 connections. This caused connection pool exhaustion ("Timeout waiting for connection from pool") during table creation/commits, manifest cleanup, data file operations, and background tasks.

Fixes #4704

The settings were only wired into the SdkHttpClient produced for StsClientsPool (used by credential vending / AwsCredentialsStorageIntegration).

All real table I/O goes through Iceberg S3FileIO:

DefaultFileIOFactory.loadFileIO → CatalogUtil.loadFileIO(..., "org.apache.iceberg.aws.s3.S3FileIO", properties) → Iceberg S3FileIO / DefaultAwsClientFactory → HttpClientProperties.applyHttpClientConfigurations → HttpClientCache → Apache HTTP client → ExceptionMappingFileIO → catalog commits / task cleanup

No http-client.apache.* properties were ever placed into the map passed to Iceberg, so it always built clients with SDK defaults (visible via Arthas as MaxConnections=50 inside the ManagedHttpClient).

vigneshio · 2026-07-08T13:49:16Z

+
+    // Apply polaris.storage.* HTTP client settings to Iceberg S3FileIO (and other AWS FileIOs).
+    // Previously only applied to the STS client pool.
+    if (s3AccessConfig != null) {


LGTM overall. 👀

The null guard here has the same effect as above.. the new HTTP settings logic gets skipped for anyone using the no-arg constructor (WasbTranslatingFileIOFactory, MeasuredFileIOFactory, and the test spy all go through that path). This means the fix is really only active for the CDI-created "default" instance.

It might be worth clarifying in the code comment that this fix targets the CDI "default" path specifically - not every place FileIOFactory gets instantiated. Just helps set the right expectations and avoids someone assuming it's a global fix.

agreed. updated the comments

Wire S3AccessConfig into WasbTranslatingFileIOFactory so production non-default factories also propagate polaris.storage.* HTTP settings to S3FileIO. Clarify that the no-arg DefaultFileIOFactory path is for tests and fixtures only.

vigneshio · 2026-07-09T11:27:38Z

LGTM.. Thanks for the quick follow-up! cc: @dimas-b

dimas-b · 2026-07-10T18:13:15Z

Are we fixing #4704 with this PR?

dimas-b

Thanks for contributing this PR, @iprithv ! I think it is an incremental improvement, but I wonder if we can take it one step further 🤔 more specific comment below.

dimas-b · 2026-07-10T18:29:47Z

+    if (s3AccessConfig != null) {
+      s3AccessConfig
+          .maxHttpConnections()
+          .ifPresent(v -> props.put("http-client.apache.max-connections", String.valueOf(v)));


This will configure the Http Client inside the FileIO with the same parameters as the Http Client used by our StsClientsPool. This is good, but it will be another Http Client instance.

However, ideally we should probably reuse the same client that is produced by sdkHttpClient().

Would that be possible?

Cf.

polaris/runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java

Line 246 in 896841e

public SdkHttpClient sdkHttpClient(S3AccessConfig config) {

Possible, but not via properties alone — Iceberg only configures HTTP clients from http-client.apache.* strings; it does not accept an external SdkHttpClient.

Reusing the singleton would need a custom AwsClientFactory (or custom S3 client supplier) wired to ServiceProducers.sdkHttpClient(), plus lifecycle care so FileIO close does not dispose the shared client (httpClient(instance) is caller-owned).

Trade-off: STS and S3 FileIO would then compete for one connection pool under load. This PR keeps separate pools with the same polaris.storage.* tuning (Iceberg already shares among FileIOs via HttpClientCache). Suggest treating full reuse as a follow-up unless you prefer expanding this PR.

thanks for checking! on reusing sdkHttpClient(), Iceberg’s FileIO path only takes http-client.apache.* properties, so we can’t pass the existing client through...sharing would need a custom AwsClientFactory (or S3 client supplier) wired to the singleton, plus care so FileIO close doesn’t dispose it.
also STS and S3 FileIO would then share one connection pool and could contend under load.

here the same polaris.storage.* settings to both I did, but kept separate clients (Iceberg still shares among FileIOs via HttpClientCache).

happy to follow up on true reuse. wdyt?

Fair enough. Thanks for the info 👍

iprithv · 2026-07-10T20:25:05Z

Are we fixing #4704 with this PR?

Yes, this is intended to fix #4704 :)

dimas-b · 2026-07-10T20:37:33Z


    // Get subcoped creds
-    properties = new HashMap<>(properties);
+    Map<String, String> props = new HashMap<>(properties);


I'd rather rename the method parameter as opposed to the local var to minimize non-functional changes below.

Why do we need to use a different variable for this anyway?

sure, renamed the parameter to tableProperties and kept the local as properties so the rest of the method stays the same.

dimas-b · 2026-07-10T20:46:16Z

+    // Apply polaris.storage.* HTTP client settings to Iceberg S3FileIO (and other AWS FileIOs).
+    // Previously only applied to the STS client pool. Skipped when constructed without config
+    // (tests / fixtures); production CDI paths always inject S3AccessConfig.
+    if (s3AccessConfig != null) {


It should be easy to add @PolarisImmutable to S3AccessConfig and create a default (empty) instance from it to avoid null config.

Only one constructor should be sufficient for all use cases, I guess... WDYT?

yes, makes sense.. added @PolarisImmutable on S3AccessConfig with S3AccessConfig.empty(), dropped the no-arg constructor, and always take a config.. tests/fixtures use S3AccessConfig.empty() (empty optionals → no HTTP props applied)

dimas-b · 2026-07-10T20:49:49Z

+      s3AccessConfig
+          .readTimeout()
+          .ifPresent(
+              d -> props.put("http-client.apache.socket-timeout-ms", String.valueOf(d.toMillis())));


Could we reuse property names from Iceberg's HttpClientProperties?

sure, done. thanks!

Address review feedback: use S3AccessConfig.empty() via @PolarisImmutable instead of null, rename loadFileIO tableProperties param, and reuse Iceberg HttpClientProperties constants. Keep regenerated polaris.storage config docs in the same commit for CI.

dimas-b

Thanks, @iprithv !

dimas-b · 2026-07-10T22:25:20Z

Targeting Jul 13 for the merge date, unless fresh comments appear.

nandorKollar

Overall the change looks good, my only concern is that now S3-specific configs are leaking into the otherwise storage-agnostic DefaultFileIOFactory. I doubt that this causes real-word problems, though look a bit awkward. Are these configs applied now on Azure and GCP storage too? What happens, when S3 is not used at all?

dimas-b · 2026-07-13T17:10:30Z

Overall the change looks good, my only concern is that now S3-specific configs are leaking into the otherwise storage-agnostic DefaultFileIOFactory

I noticed that too. My reading that such a leak existed before too, but was realized in untyped FileIO property bags. This PR merely makes it more visible by using a strongly typed config class. All in all, I think it's fine for now.

dimas-b · 2026-07-13T17:12:12Z

What happens, when S3 is not used at all?

The S3 config will be "empty". It is ultimately produced by Quarkus at the server start time (once per instance) and should not cause too much overhead.

nandorKollar · 2026-07-13T20:46:20Z

Overall the change looks good, my only concern is that now S3-specific configs are leaking into the otherwise storage-agnostic DefaultFileIOFactory

I noticed that too. My reading that such a leak existed before too, but was realized in untyped FileIO property bags. This PR merely makes it more visible by using a strongly typed config class. All in all, I think it's fine for now.

Thanks @dimas-b for the explanation. Agree, this is fine for now.

nandorKollar · 2026-07-14T14:43:30Z

@iprithv would you mind resolve the merge conflict on the changelog?

iprithv · 2026-07-14T19:54:30Z

Is it only for me? I don't see any conflict on this 😅

dimas-b · 2026-07-15T15:57:33Z

I guess someone changed CHANGELOG on main at the conflict was gone 😉

Propagate storage HTTP client settings to S3FileIO for table operations

a5ce9af

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 8, 2026

github-project-automation Bot added this to Basic Kanban Board Jul 8, 2026

vigneshio reviewed Jul 8, 2026

View reviewed changes

iprithv requested a review from vigneshio July 9, 2026 05:36

vigneshio approved these changes Jul 9, 2026

View reviewed changes

dimas-b reviewed Jul 10, 2026

View reviewed changes

iprithv force-pushed the fix-s3-http-client-settings branch from db64ba2 to f5b1118 Compare July 10, 2026 20:32

dimas-b reviewed Jul 10, 2026

View reviewed changes

review changes

6f0fd02

Address review feedback: use S3AccessConfig.empty() via @PolarisImmutable instead of null, rename loadFileIO tableProperties param, and reuse Iceberg HttpClientProperties constants. Keep regenerated polaris.storage config docs in the same commit for CI.

iprithv force-pushed the fix-s3-http-client-settings branch from f5b1118 to 6f0fd02 Compare July 10, 2026 20:58

iprithv requested a review from dimas-b July 10, 2026 21:24

dimas-b approved these changes Jul 10, 2026

View reviewed changes

github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 10, 2026

nandorKollar approved these changes Jul 13, 2026

View reviewed changes

vigneshio approved these changes Jul 14, 2026

View reviewed changes

nandorKollar merged commit 26a33dc into apache:main Jul 15, 2026
25 checks passed

github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Jul 15, 2026

andrii29 mentioned this pull request Jul 18, 2026

Deleting Large Tables causes the system to exhaust connections pool or become unresponsive. #3462

Open

Uh oh!

Conversation

iprithv commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vigneshio commented Jul 9, 2026

Uh oh!

dimas-b commented Jul 10, 2026

Uh oh!

dimas-b 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!

iprithv Jul 10, 2026 • edited 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!

iprithv commented Jul 10, 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!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Jul 10, 2026

Uh oh!

nandorKollar left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Jul 13, 2026

Uh oh!

dimas-b commented Jul 13, 2026

Uh oh!

nandorKollar commented Jul 13, 2026

Uh oh!

nandorKollar commented Jul 14, 2026

Uh oh!

iprithv commented Jul 14, 2026

Uh oh!

Uh oh!

dimas-b commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iprithv commented Jul 8, 2026 •

edited

Loading

iprithv Jul 10, 2026 •

edited

Loading