kevinjqliu commented on code in PR #16258:
URL: https://github.com/apache/iceberg/pull/16258#discussion_r3211823660
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/BaseGCSFile.java:
##########
@@ -28,15 +27,15 @@
abstract class BaseGCSFile {
private final Storage storage;
- private final GcsFileSystem gcsFileSystem;
Review Comment:
is it better to use `AutoCloseable` instead of `Object`? (this is a claude
suggestion)
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/PrefixedStorage.java:
##########
@@ -59,6 +55,11 @@ class PrefixedStorage implements AutoCloseable {
this.storagePrefix = storagePrefix;
this.storage = storage;
this.gcpProperties = new GCPProperties(properties);
+ this.propertiesWithUserAgent =
+ ImmutableMap.<String, String>builder()
+ .putAll(properties)
+ .put("gcs.user-agent", GCS_FILE_IO_USER_AGENT)
+ .build();
Review Comment:
nit: If the caller's properties already contains "gcs.user-agent",
ImmutableMap.Builder.put() will throw IllegalArgumentException for duplicate
keys. This existed in the old code too, but now the map is built eagerly
(always) rather than lazily (only when creating the supplier). Consider using
buildKeepingLast() or filtering the key before putAll.
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/PrefixedStorage.java:
##########
@@ -115,23 +114,33 @@ public void close() {
}
}
+ if (null != gcsFileSystem) {
+ AnalyticsCoreUtil.close(gcsFileSystem);
+ gcsFileSystem = null;
+ }
+
Review Comment:
If AnalyticsCoreUtil.close() throws (e.g., the underlying
GcsFileSystem.close() fails), the storage = null cleanup is skipped. The old
code added gcsFileSystem to closeableGroup, which handles multi-resource
cleanup safely. Consider wrapping in try-finally
--
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]
