GCP: Add ability to configure connect and read timeouts for GCS HTTP client by umatt1 · Pull Request #17786 · apache/iceberg

umatt1 · 2026-08-24T00:18:26Z

Motivation

GCSFileIO always builds its Storage client with the library's default HTTP transport timeouts, which are effectively unbounded for reads (java.net.URLConnection blocks indefinitely by default). There is no way to tune this for degraded network conditions. The AWS module already exposes the equivalent knobs via HttpClientProperties (http-client.*-timeout-ms); this closes that gap for GCS.

A prior PR for this issue (#15626) went stale from a rebase gap rather than any design objection — reviewers explicitly pushed back on the stale-close at the time. This PR is scoped narrower: only the timeout configuration requested in #15587, without the unrelated #15411 changes that PR also carried.

Changes

Two new optional properties, following the existing GCPProperties conventions:

gcs.http.connect-timeout-ms
gcs.http.read-timeout-ms

When either is set, PrefixedStorage builds an HttpTransportOptions and wires it into the StorageOptions.Builder, in the same place serviceHost/projectId are already applied. When unset, behavior is byte-for-byte identical to before (no setTransportOptions call is made). Purely additive — no signature or behavior changes for existing users.

One behavior worth reviewer attention, documented in the Javadoc: these timeouts apply per HTTP attempt, not per overall call. The Storage client retries failed requests with backoff by default (gax default total retry timeout ~50s), so a configured read timeout bounds each attempt, not total wall time.

Testing

TestGCPProperties: properties parse correctly and remain unset by default
TestPrefixedStorage#httpTimeoutsAreWired / #httpTimeoutsNotSetByDefault: values land on the real HttpTransportOptions of the constructed client; defaults untouched when unset
TestPrefixedStorage#readTimeoutIsActuallyEnforced: functional test against a local socket that accepts the connection but never responds — the request fails with Read timed out / SocketTimeoutException in ~1s instead of hanging on the unbounded default. Uses maxAttempts(1) to isolate a single attempt from the default retry policy (which is what surfaced the per-attempt semantics above).

./gradlew :iceberg-gcp:check passes (tests, checkstyle, spotless).

AI disclosure

This PR was developed with AI assistance (Claude Code), per the AI-assisted contribution guidelines. All code and tests were run and verified locally; the per-attempt timeout semantics called out above were discovered through the functional test rather than assumed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WVZAaD5G3sWVKsFi8sYDE9

…ient GCSOutputStream/GCSFileIO always used the client library's default HTTP transport timeouts, with no way to tune them for network conditions. This adds two optional properties, gcs.http.connect-timeout-ms and gcs.http.read-timeout-ms, wired into the underlying HttpTransportOptions used to build the GCS Storage client. Closes apache#15587

The existing tests only verified the configured timeout values are stored on the built client. This adds a test that starts a local socket accepting connections but never responding, confirming a request against it actually fails around the configured read timeout instead of hanging on the SDK's effectively-unbounded default. The Storage client's default retry policy (~50s total) swamps a single request's timeout, so the test uses maxAttempts(1) to isolate one HTTP attempt, which is what the timeout property governs.

assertThatThrownBy must include a message check per this repo's custom checkstyle rule. Assert on the exact message and cause type, which also strengthens the test: it confirms the failure is genuinely a read timeout (SocketTimeoutException), not a connect failure or other error that happened to also be fast.

The contributing guide asks new test methods to omit the public modifier (JUnit 5 discovers package-private test methods fine). The existing methods in these files predate that convention and are left as-is to avoid unrelated churn.

Testing surfaced non-obvious behavior worth calling out in the Javadoc: the Storage client retries failed requests by default, so these timeouts bound a single HTTP attempt, not overall call latency. Without this note a user could reasonably expect the configured value to cap total wall time.

ebyhr · 2026-08-24T00:42:51Z

+    if (properties.containsKey(GCS_HTTP_CONNECT_TIMEOUT)) {
+      gcsHttpConnectTimeoutMs = Integer.parseInt(properties.get(GCS_HTTP_CONNECT_TIMEOUT));
+    }
+
+    if (properties.containsKey(GCS_HTTP_READ_TIMEOUT)) {
+      gcsHttpReadTimeoutMs = Integer.parseInt(properties.get(GCS_HTTP_READ_TIMEOUT));
+    }


Can we use propertyAsNullableInt helper method instead?

Done in 8bf6b03 — both timeouts now use PropertyUtil.propertyAsNullableInt.

ebyhr · 2026-08-24T00:46:18Z

+            if (gcpProperties.httpConnectTimeoutMs().isPresent()
+                || gcpProperties.httpReadTimeoutMs().isPresent()) {
+              HttpTransportOptions.Builder transportBuilder = HttpTransportOptions.newBuilder();
+              gcpProperties.httpConnectTimeoutMs().ifPresent(transportBuilder::setConnectTimeout);
+              gcpProperties.httpReadTimeoutMs().ifPresent(transportBuilder::setReadTimeout);
+              builder.setTransportOptions(transportBuilder.build());
+            }


Can we remove the if condition?

HttpTransportOptions.Builder transportBuilder = HttpTransportOptions.newBuilder(); gcpProperties.httpConnectTimeoutMs().ifPresent(transportBuilder::setConnectTimeout); gcpProperties.httpReadTimeoutMs().ifPresent(transportBuilder::setReadTimeout); builder.setTransportOptions(transportBuilder.build());

Done in 8bf6b03. Confirmed it's behavior-preserving: the httpTimeoutsNotSetByDefault test asserts the resulting timeouts match HttpTransportOptions.newBuilder().build(), so always setting an explicitly-default-built transport is equivalent to the library's implicit one.

ebyhr · 2026-08-24T00:47:59Z

+  }
+
+  @Test
+  void readTimeoutIsActuallyEnforced() throws IOException {


The code comments in this test look too verbose. I recommend simplifying the code comment.

Trimmed twice — first in 8bf6b03, then further in 5f998b7 after checking the module's actual convention. The other test files here use single-line comments at the point of use (max two lines anywhere in the gcp test module), and TestPrefixedStorage had none at all before this PR. So the three-line preamble is gone, replaced by two single-line comments next to the code they describe:

// accepts the connection but never responds, so the read blocks until the timeout fires

// isolate a single attempt, since the timeout applies per attempt and not to the retry loop

Happy to cut the second one too if you'd rather — it's the only non-obvious bit (maxAttempts(1) looks arbitrary without it).

ebyhr · 2026-08-24T00:49:54Z

+    assertThat(gcpProperties.httpConnectTimeoutMs()).isPresent().get().isEqualTo(5000);
+    assertThat(gcpProperties.httpReadTimeoutMs()).isPresent().get().isEqualTo(10000);


AssertJ provides contains and hasValue method to verify the value in a optional type:

Suggested change

assertThat(gcpProperties.httpConnectTimeoutMs()).isPresent().get().isEqualTo(5000);

assertThat(gcpProperties.httpReadTimeoutMs()).isPresent().get().isEqualTo(10000);

assertThat(gcpProperties.httpConnectTimeoutMs()).hasValue(5000);

assertThat(gcpProperties.httpReadTimeoutMs()).hasValue(10000);

Done in 8bf6b03 — switched to hasValue().

- Use PropertyUtil.propertyAsNullableInt for timeout parsing - Always set transport options instead of guarding on presence - Use AssertJ hasValue() for Optional assertions - Trim verbose comments in the read-timeout test

umatt1 · 2026-08-24T01:22:01Z

Thanks for the review @ebyhr! Addressed all four points in 8bf6b03:

PropertyUtil.propertyAsNullableInt for both timeout properties
Removed the presence guard — transport options are now always set (verified the httpTimeoutsNotSetByDefault test still passes, since newBuilder().build() matches the library defaults)
hasValue() for the Optional assertions
Trimmed the test comments down to the two things a future reader needs (why the unresponsive server, why maxAttempts(1))

Replaces the three-line preamble with single-line comments next to the code each describes, matching the comment style used elsewhere in the gcp test module.

uros-b · 2026-08-24T09:12:35Z

+1, thank you @umatt1 and @ebyhr!

umatt1 added 5 commits August 23, 2026 18:33

umatt1 mentioned this pull request Aug 24, 2026

Add ability to configure connection timeout and read timeout while reading google cloud storage objects in Iceberg. #15587

Open

3 tasks

github-actions Bot added the GCP label Aug 24, 2026

ebyhr reviewed Aug 24, 2026

View reviewed changes

GCP: Address review feedback

8bf6b03

- Use PropertyUtil.propertyAsNullableInt for timeout parsing - Always set transport options instead of guarding on presence - Use AssertJ hasValue() for Optional assertions - Trim verbose comments in the read-timeout test

GCP: Move test comments inline at point of use

5f998b7

Replaces the three-line preamble with single-line comments next to the code each describes, matching the comment style used elsewhere in the gcp test module.

uros-b approved these changes Aug 24, 2026

View reviewed changes

umatt1 requested a review from ebyhr August 24, 2026 14:01

Navigation Menu

Uh oh!

GCP: Add ability to configure connect and read timeouts for GCS HTTP client - #17786

GCP: Add ability to configure connect and read timeouts for GCS HTTP client#17786
umatt1 wants to merge 7 commits into
apache:mainfrom
umatt1:gcs-http-timeout-config

umatt1 commented Aug 24, 2026

Uh oh!

ebyhr Aug 24, 2026

Uh oh!

umatt1 Aug 24, 2026

Uh oh!

ebyhr Aug 24, 2026

Uh oh!

umatt1 Aug 24, 2026

Uh oh!

ebyhr Aug 24, 2026

Uh oh!

umatt1 Aug 24, 2026

Uh oh!

ebyhr Aug 24, 2026

Uh oh!

umatt1 Aug 24, 2026

Uh oh!

umatt1 commented Aug 24, 2026

Uh oh!

uros-b commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		assertThat(gcpProperties.httpConnectTimeoutMs()).isPresent().get().isEqualTo(5000);
		assertThat(gcpProperties.httpReadTimeoutMs()).isPresent().get().isEqualTo(10000);

Uh oh!

Conversation

umatt1 commented Aug 24, 2026

Motivation

Changes

Testing

AI disclosure

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umatt1 commented Aug 24, 2026

Uh oh!

uros-b commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants