umatt1 commented on code in PR #17786:
URL: https://github.com/apache/iceberg/pull/17786#discussion_r3840195395
##########
gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java:
##########
@@ -149,6 +163,14 @@ public GCPProperties(Map<String, String> properties) {
clientLibToken = properties.get(GCS_CLIENT_LIB_TOKEN);
serviceHost = properties.get(GCS_SERVICE_HOST);
+ 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));
+ }
Review Comment:
Done in 8bf6b03c7 — both timeouts now use
`PropertyUtil.propertyAsNullableInt`.
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/PrefixedStorage.java:
##########
@@ -74,6 +75,14 @@ class PrefixedStorage implements AutoCloseable {
gcpProperties.clientLibToken().ifPresent(builder::setClientLibToken);
gcpProperties.serviceHost().ifPresent(builder::setHost);
+ 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());
+ }
Review Comment:
Done in 8bf6b03c7. 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.
##########
gcp/src/test/java/org/apache/iceberg/gcp/TestGCPProperties.java:
##########
@@ -77,4 +79,22 @@ public void
refreshCredentialsEndpointSetButRefreshDisabled() {
.get()
.isEqualTo("/v1/credentials");
}
+
+ @Test
+ void httpTimeoutsNotSetByDefault() {
+ GCPProperties gcpProperties = new GCPProperties(ImmutableMap.of());
+ assertThat(gcpProperties.httpConnectTimeoutMs()).isNotPresent();
+ assertThat(gcpProperties.httpReadTimeoutMs()).isNotPresent();
+ }
+
+ @Test
+ void httpTimeoutsAreRead() {
+ GCPProperties gcpProperties =
+ new GCPProperties(
+ ImmutableMap.of(
+ GCS_HTTP_CONNECT_TIMEOUT, "5000",
+ GCS_HTTP_READ_TIMEOUT, "10000"));
+
assertThat(gcpProperties.httpConnectTimeoutMs()).isPresent().get().isEqualTo(5000);
+
assertThat(gcpProperties.httpReadTimeoutMs()).isPresent().get().isEqualTo(10000);
Review Comment:
Done in 8bf6b03c7 — switched to `hasValue()`.
##########
gcp/src/test/java/org/apache/iceberg/gcp/gcs/TestPrefixedStorage.java:
##########
@@ -75,6 +83,94 @@ public void userAgentPrefix() {
.isEqualTo("gcsfileio/" + EnvironmentContext.get());
}
+ @Test
+ void httpTimeoutsNotSetByDefault() {
+ Map<String, String> properties =
ImmutableMap.of(GCPProperties.GCS_PROJECT_ID, "myProject");
+ PrefixedStorage storage = new PrefixedStorage("gs://bucket", properties,
null);
+
+ assertThat(storage.storage().getOptions().getTransportOptions())
+ .isInstanceOf(HttpTransportOptions.class);
+ HttpTransportOptions transportOptions =
+ (HttpTransportOptions)
storage.storage().getOptions().getTransportOptions();
+ assertThat(transportOptions.getConnectTimeout())
+
.isEqualTo(HttpTransportOptions.newBuilder().build().getConnectTimeout());
+ assertThat(transportOptions.getReadTimeout())
+ .isEqualTo(HttpTransportOptions.newBuilder().build().getReadTimeout());
+ }
+
+ @Test
+ void httpTimeoutsAreWired() {
+ Map<String, String> properties =
+ ImmutableMap.of(
+ GCPProperties.GCS_PROJECT_ID, "myProject",
+ GCPProperties.GCS_HTTP_CONNECT_TIMEOUT, "5000",
+ GCPProperties.GCS_HTTP_READ_TIMEOUT, "10000");
+ PrefixedStorage storage = new PrefixedStorage("gs://bucket", properties,
null);
+
+ HttpTransportOptions transportOptions =
+ (HttpTransportOptions)
storage.storage().getOptions().getTransportOptions();
+ assertThat(transportOptions.getConnectTimeout()).isEqualTo(5000);
+ assertThat(transportOptions.getReadTimeout()).isEqualTo(10000);
+ }
+
+ @Test
+ void readTimeoutIsActuallyEnforced() throws IOException {
Review Comment:
Trimmed twice — first in 8bf6b03c7, then further in 5f998b7a8 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).
--
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]
