From 32609b89b6551683009a3bf8b30c636670daf0e1 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 10 Jun 2026 11:40:57 +0000 Subject: [PATCH 01/28] feat: Strategy Interface Definition and Implementation --- .../namespace/FlatNamespaceStrategyImpl.java | 49 +++++++++++++++++++ .../HierarchicalNamespaceStrategyImpl.java | 49 +++++++++++++++++++ .../client/namespace/NamespaceStrategy.java | 34 +++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java new file mode 100644 index 000000000..55e535471 --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java @@ -0,0 +1,49 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client.namespace; + +import com.google.cloud.gcs.analyticscore.client.GcsItemId; +import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; +import com.google.cloud.gcs.analyticscore.common.PathType; +import java.io.IOException; + +public class FlatNamespaceStrategyImpl implements NamespaceStrategy { + @Override + public GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void mkdirs(GcsItemId id) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void delete(GcsItemId id, boolean recursive) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void rename(GcsItemId src, GcsItemId dst) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public java.util.List listStatus(GcsItemId id) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } +} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java new file mode 100644 index 000000000..64e790dbb --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java @@ -0,0 +1,49 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client.namespace; + +import com.google.cloud.gcs.analyticscore.client.GcsItemId; +import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; +import com.google.cloud.gcs.analyticscore.common.PathType; +import java.io.IOException; + +public class HierarchicalNamespaceStrategyImpl implements NamespaceStrategy { + @Override + public GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void mkdirs(GcsItemId id) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void delete(GcsItemId id, boolean recursive) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void rename(GcsItemId src, GcsItemId dst) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public java.util.List listStatus(GcsItemId id) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } +} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java new file mode 100644 index 000000000..a61feb474 --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java @@ -0,0 +1,34 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client.namespace; + +import com.google.cloud.gcs.analyticscore.client.GcsItemId; +import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; +import com.google.cloud.gcs.analyticscore.common.PathType; +import java.io.IOException; + +public interface NamespaceStrategy { + GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException; + + void mkdirs(GcsItemId id) throws IOException; + + void delete(GcsItemId id, boolean recursive) throws IOException; + + void rename(GcsItemId src, GcsItemId dst) throws IOException; + + java.util.List listStatus(GcsItemId id) throws IOException; +} From daaeb1a50986f9c1ce1bfef01cdfa9822180bf07 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 10 Jun 2026 11:41:15 +0000 Subject: [PATCH 02/28] feat: File System Integration and Routing Mechanics --- .../analyticscore/client/GcsFileSystem.java | 78 ++++++++++++++++++ .../client/GcsFileSystemImpl.java | 81 +++++++++++++++++++ .../client/GcsFileSystemOptions.java | 33 ++++++++ 3 files changed, 192 insertions(+) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java index 4a3411e21..b1be787c5 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java @@ -15,11 +15,13 @@ */ package com.google.cloud.gcs.analyticscore.client; +import com.google.cloud.gcs.analyticscore.common.PathType; import com.google.cloud.gcs.analyticscore.common.telemetry.Telemetry; import java.io.FileNotFoundException; import java.io.IOException; import java.net.URI; import java.nio.channels.WritableByteChannel; +import java.util.Map; public interface GcsFileSystem extends AutoCloseable { @@ -57,6 +59,82 @@ VectoredSeekableByteChannel open(GcsFileInfo gcsFileInfo, GcsReadOptions options /** Gets Metadata about the given gcs object represented by itemId. */ GcsFileInfo getFileInfo(GcsItemId itemId) throws IOException; + /** + * Gets Metadata about the given GCS object, interpreting it as the specified PathType. + * + * @param itemId The identifier of the GCS object. + * @param pathType The type of path (e.g., file or directory). + * @return Metadata about the given path item. + */ + GcsItemInfo getFileInfo(GcsItemId itemId, PathType pathType) throws IOException; + + /** + * Lists the statuses of the files/directories in the given path. + * + * @param path The path we want to list. + * @return A list of GcsFileInfo. + */ + java.util.List listStatus(URI path) throws IOException; + + /** + * Lists the statuses of the files/directories in the given path. + * + * @param itemId The identifier of the given path. + * @return A list of GcsFileInfo. + */ + java.util.List listStatus(GcsItemId itemId) throws IOException; + + /** + * Creates the directory named by the given identifier, including any necessary but nonexistent + * parent directories. + * + * @param id The identifier for the directory to create. + */ + void mkdirs(GcsItemId id) throws IOException; + + /** + * Deletes the item denoted by the given identifier. If recursive is true and the item is a + * directory, all contents will be deleted. + * + * @param id The identifier of the item to delete. + * @param recursive Whether to recursively delete contents if the item is a directory. + */ + void delete(GcsItemId id, boolean recursive) throws IOException; + + /** + * Renames the item from the source identifier to the destination identifier. + * + * @param src The current identifier of the item. + * @param dst The new identifier for the item. + */ + void rename(GcsItemId src, GcsItemId dst) throws IOException; + + /** + * Retrieves the value of an extended attribute (custom metadata) for the given item. + * + * @param id The identifier of the item. + * @param name The name of the extended attribute. + * @return The byte array value of the extended attribute, or null if it does not exist. + */ + byte[] getXAttr(GcsItemId id, String name) throws IOException; + + /** + * Sets an extended attribute (custom metadata) for the given item. + * + * @param id The identifier of the item. + * @param name The name of the extended attribute. + * @param value The byte array value to set for the attribute. + */ + void setXAttr(GcsItemId id, String name, byte[] value) throws IOException; + + /** + * Retrieves all extended attributes (custom metadata) for the given item. + * + * @param id The identifier of the item. + * @return A map of extended attribute names to their byte array values. + */ + Map getXAttrs(GcsItemId id) throws IOException; + /** Retrieve the options that were used to create this GcsFileSystem. */ GcsFileSystemOptions getFileSystemOptions(); diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 322e68d16..4a86b81ec 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -19,6 +19,11 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.auth.Credentials; +import com.google.cloud.gcs.analyticscore.client.cache.BucketCapabilitiesCache; +import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; +import com.google.cloud.gcs.analyticscore.common.BucketCapabilities; import com.google.cloud.gcs.analyticscore.common.GcsAnalyticsCoreTelemetryConstants; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -51,6 +56,10 @@ public class GcsFileSystemImpl implements GcsFileSystem { private final Telemetry telemetry; private final AnalyticsCacheManager cacheManager; + private final FlatNamespaceStrategyImpl flatStrategy = new FlatNamespaceStrategyImpl(); + private final HierarchicalNamespaceStrategyImpl hnsStrategy = + new HierarchicalNamespaceStrategyImpl(); + public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { this.fileSystemOptions = fileSystemOptions; this.executorServiceSupplier = initializeExecutionServiceSupplier(); @@ -104,6 +113,15 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = telemetry; this.cacheManager = cacheManager; + + public NamespaceStrategy resolveStrategy(String bucketName) throws IOException { + BucketCapabilities capabilities = + cacheManager.getBucketCapabilities(bucketName, gcsClient::getBucketCapabilities); + + if (capabilities.isHnsEnabled() && fileSystemOptions.isHnsApiEnabled()) { + return hnsStrategy; + } + return flatStrategy; } @Override @@ -143,6 +161,69 @@ public GcsFileInfo getFileInfo(GcsItemId itemId) throws IOException { .build(); } + @Override + public GcsItemInfo getFileInfo( + GcsItemId itemId, com.google.cloud.gcs.analyticscore.common.PathType pathType) + throws IOException { + return resolveStrategy(itemId.getBucketName()).getFileInfo(itemId, pathType); + } + + @Override + public java.util.List listStatus(URI path) throws IOException { + GcsItemId itemId = UriUtil.getItemIdFromString(path.toString()); + return listStatus(itemId); + } + + @Override + public java.util.List listStatus(GcsItemId itemId) throws IOException { + java.util.List itemInfos = + resolveStrategy(itemId.getBucketName()).listStatus(itemId); + return itemInfos.stream() + .map( + info -> + GcsFileInfo.builder() + .setItemInfo(info) + .setUri( + URI.create( + BlobId.of( + info.getItemId().getBucketName(), + info.getItemId().getObjectName().get()) + .toGsUtilUri())) + .setAttributes(Collections.emptyMap()) + .build()) + .collect(java.util.stream.Collectors.toList()); + } + + @Override + public void mkdirs(GcsItemId id) throws IOException { + resolveStrategy(id.getBucketName()).mkdirs(id); + } + + @Override + public void delete(GcsItemId id, boolean recursive) throws IOException { + resolveStrategy(id.getBucketName()).delete(id, recursive); + } + + @Override + public void rename(GcsItemId src, GcsItemId dst) throws IOException { + resolveStrategy(src.getBucketName()).rename(src, dst); + } + + @Override + public byte[] getXAttr(GcsItemId id, String name) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public void setXAttr(GcsItemId id, String name, byte[] value) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + + @Override + public java.util.Map getXAttrs(GcsItemId id) throws IOException { + throw new UnsupportedOperationException("Not implemented yet"); + } + @Override public GcsFileSystemOptions getFileSystemOptions() { return this.fileSystemOptions; diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java index 95ab244cf..58c8b33b7 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java @@ -25,6 +25,11 @@ public abstract class GcsFileSystemOptions { private static final String READ_THREAD_COUNT_KEY = "analytics-core.read.thread.count"; private static final String CLIENT_TYPE_KEY = "client.type"; + private static final String HNS_API_ENABLED_KEY = "analytics-core.hns.api.enable"; + private static final String BUCKET_CAPABILITY_CACHE_TIMEOUT_MINUTES_KEY = + "analytics-core.bucket.capability.cache.timeout.minutes"; + private static final String BUCKET_CAPABILITY_CACHE_MAX_SIZE_KEY = + "analytics-core.bucket.capability.cache.max.size"; /** Cloud Storage client to use. */ public enum ClientType { @@ -43,12 +48,21 @@ public enum ClientType { public abstract TelemetryOptions getAnalyticsCoreTelemetryOptions(); + public abstract boolean isHnsApiEnabled(); + + public abstract long getBucketCapabilityCacheTimeoutMinutes(); + + public abstract long getBucketCapabilityCacheMaxSize(); + public abstract Builder toBuilder(); public static Builder builder() { return new AutoValue_GcsFileSystemOptions.Builder() .setReadThreadCount(16) .setClientType(ClientType.HTTP_CLIENT) + .setHnsApiEnabled(false) + .setBucketCapabilityCacheTimeoutMinutes(5L) + .setBucketCapabilityCacheMaxSize(1000L) .setGcsClientOptions(GcsClientOptions.builder().build()) .setGcsCacheOptions(GcsCacheOptions.builder().build()) .setAnalyticsCoreTelemetryOptions(TelemetryOptions.builder().build()); @@ -65,6 +79,19 @@ public static GcsFileSystemOptions createFromOptions( optionsBuilder.setClientType( ClientType.valueOf(analyticsCoreOptions.get(prefix + CLIENT_TYPE_KEY))); } + if (analyticsCoreOptions.containsKey(prefix + HNS_API_ENABLED_KEY)) { + optionsBuilder.setHnsApiEnabled( + Boolean.parseBoolean(analyticsCoreOptions.get(prefix + HNS_API_ENABLED_KEY))); + } + if (analyticsCoreOptions.containsKey(prefix + BUCKET_CAPABILITY_CACHE_TIMEOUT_MINUTES_KEY)) { + optionsBuilder.setBucketCapabilityCacheTimeoutMinutes( + Long.parseLong( + analyticsCoreOptions.get(prefix + BUCKET_CAPABILITY_CACHE_TIMEOUT_MINUTES_KEY))); + } + if (analyticsCoreOptions.containsKey(prefix + BUCKET_CAPABILITY_CACHE_MAX_SIZE_KEY)) { + optionsBuilder.setBucketCapabilityCacheMaxSize( + Long.parseLong(analyticsCoreOptions.get(prefix + BUCKET_CAPABILITY_CACHE_MAX_SIZE_KEY))); + } optionsBuilder.setGcsClientOptions( GcsClientOptions.createFromOptions(analyticsCoreOptions, prefix)); optionsBuilder.setGcsCacheOptions( @@ -84,6 +111,12 @@ public abstract static class Builder { public abstract Builder setReadThreadCount(int readThreadCount); + public abstract Builder setHnsApiEnabled(boolean isHnsApiEnabled); + + public abstract Builder setBucketCapabilityCacheTimeoutMinutes(long timeout); + + public abstract Builder setBucketCapabilityCacheMaxSize(long maxSize); + public abstract Builder setGcsClientOptions(GcsClientOptions gcsClientOptions); /** Sets the configuration options for the GCS caching layer. */ From e53dea2eed55d46332d8f63b4afdb79dcc07d3af Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 16 Jun 2026 11:14:26 +0000 Subject: [PATCH 03/28] refactor: fix import paths and rmeove redundant gcsFilesystemOptions --- .../analyticscore/client/GcsFileSystem.java | 1 - .../client/GcsFileSystemImpl.java | 7 ++---- .../client/GcsFileSystemOptions.java | 24 +------------------ .../namespace/FlatNamespaceStrategyImpl.java | 2 +- .../HierarchicalNamespaceStrategyImpl.java | 2 +- .../client/namespace/NamespaceStrategy.java | 2 +- 6 files changed, 6 insertions(+), 32 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java index b1be787c5..ba82f5f9f 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java @@ -15,7 +15,6 @@ */ package com.google.cloud.gcs.analyticscore.client; -import com.google.cloud.gcs.analyticscore.common.PathType; import com.google.cloud.gcs.analyticscore.common.telemetry.Telemetry; import java.io.FileNotFoundException; import java.io.IOException; diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 4a86b81ec..373349d20 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -19,11 +19,9 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.auth.Credentials; -import com.google.cloud.gcs.analyticscore.client.cache.BucketCapabilitiesCache; import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; -import com.google.cloud.gcs.analyticscore.common.BucketCapabilities; import com.google.cloud.gcs.analyticscore.common.GcsAnalyticsCoreTelemetryConstants; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -113,6 +111,7 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = telemetry; this.cacheManager = cacheManager; + } public NamespaceStrategy resolveStrategy(String bucketName) throws IOException { BucketCapabilities capabilities = @@ -162,9 +161,7 @@ public GcsFileInfo getFileInfo(GcsItemId itemId) throws IOException { } @Override - public GcsItemInfo getFileInfo( - GcsItemId itemId, com.google.cloud.gcs.analyticscore.common.PathType pathType) - throws IOException { + public GcsItemInfo getFileInfo(GcsItemId itemId, PathType pathType) throws IOException { return resolveStrategy(itemId.getBucketName()).getFileInfo(itemId, pathType); } diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java index 58c8b33b7..76d09a5de 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java @@ -26,10 +26,6 @@ public abstract class GcsFileSystemOptions { private static final String READ_THREAD_COUNT_KEY = "analytics-core.read.thread.count"; private static final String CLIENT_TYPE_KEY = "client.type"; private static final String HNS_API_ENABLED_KEY = "analytics-core.hns.api.enable"; - private static final String BUCKET_CAPABILITY_CACHE_TIMEOUT_MINUTES_KEY = - "analytics-core.bucket.capability.cache.timeout.minutes"; - private static final String BUCKET_CAPABILITY_CACHE_MAX_SIZE_KEY = - "analytics-core.bucket.capability.cache.max.size"; /** Cloud Storage client to use. */ public enum ClientType { @@ -50,10 +46,6 @@ public enum ClientType { public abstract boolean isHnsApiEnabled(); - public abstract long getBucketCapabilityCacheTimeoutMinutes(); - - public abstract long getBucketCapabilityCacheMaxSize(); - public abstract Builder toBuilder(); public static Builder builder() { @@ -61,8 +53,6 @@ public static Builder builder() { .setReadThreadCount(16) .setClientType(ClientType.HTTP_CLIENT) .setHnsApiEnabled(false) - .setBucketCapabilityCacheTimeoutMinutes(5L) - .setBucketCapabilityCacheMaxSize(1000L) .setGcsClientOptions(GcsClientOptions.builder().build()) .setGcsCacheOptions(GcsCacheOptions.builder().build()) .setAnalyticsCoreTelemetryOptions(TelemetryOptions.builder().build()); @@ -83,15 +73,7 @@ public static GcsFileSystemOptions createFromOptions( optionsBuilder.setHnsApiEnabled( Boolean.parseBoolean(analyticsCoreOptions.get(prefix + HNS_API_ENABLED_KEY))); } - if (analyticsCoreOptions.containsKey(prefix + BUCKET_CAPABILITY_CACHE_TIMEOUT_MINUTES_KEY)) { - optionsBuilder.setBucketCapabilityCacheTimeoutMinutes( - Long.parseLong( - analyticsCoreOptions.get(prefix + BUCKET_CAPABILITY_CACHE_TIMEOUT_MINUTES_KEY))); - } - if (analyticsCoreOptions.containsKey(prefix + BUCKET_CAPABILITY_CACHE_MAX_SIZE_KEY)) { - optionsBuilder.setBucketCapabilityCacheMaxSize( - Long.parseLong(analyticsCoreOptions.get(prefix + BUCKET_CAPABILITY_CACHE_MAX_SIZE_KEY))); - } + optionsBuilder.setGcsClientOptions( GcsClientOptions.createFromOptions(analyticsCoreOptions, prefix)); optionsBuilder.setGcsCacheOptions( @@ -113,10 +95,6 @@ public abstract static class Builder { public abstract Builder setHnsApiEnabled(boolean isHnsApiEnabled); - public abstract Builder setBucketCapabilityCacheTimeoutMinutes(long timeout); - - public abstract Builder setBucketCapabilityCacheMaxSize(long maxSize); - public abstract Builder setGcsClientOptions(GcsClientOptions gcsClientOptions); /** Sets the configuration options for the GCS caching layer. */ diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java index 55e535471..8f729aa3d 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java @@ -18,7 +18,7 @@ import com.google.cloud.gcs.analyticscore.client.GcsItemId; import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; -import com.google.cloud.gcs.analyticscore.common.PathType; +import com.google.cloud.gcs.analyticscore.client.PathType; import java.io.IOException; public class FlatNamespaceStrategyImpl implements NamespaceStrategy { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java index 64e790dbb..bbcbebcc3 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java @@ -18,7 +18,7 @@ import com.google.cloud.gcs.analyticscore.client.GcsItemId; import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; -import com.google.cloud.gcs.analyticscore.common.PathType; +import com.google.cloud.gcs.analyticscore.client.PathType; import java.io.IOException; public class HierarchicalNamespaceStrategyImpl implements NamespaceStrategy { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java index a61feb474..009357057 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java @@ -18,7 +18,7 @@ import com.google.cloud.gcs.analyticscore.client.GcsItemId; import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; -import com.google.cloud.gcs.analyticscore.common.PathType; +import com.google.cloud.gcs.analyticscore.client.PathType; import java.io.IOException; public interface NamespaceStrategy { From 1d36799f4877319cbec5bdb59e5c2fd5d68ab8a0 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 17 Jun 2026 08:55:32 +0000 Subject: [PATCH 04/28] refactor: remove unused filesystem operations from GcsFileSystem --- .../analyticscore/client/GcsFileSystem.java | 76 ------------------- .../client/GcsFileSystemImpl.java | 64 ---------------- 2 files changed, 140 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java index ba82f5f9f..017f33607 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java @@ -58,82 +58,6 @@ VectoredSeekableByteChannel open(GcsFileInfo gcsFileInfo, GcsReadOptions options /** Gets Metadata about the given gcs object represented by itemId. */ GcsFileInfo getFileInfo(GcsItemId itemId) throws IOException; - /** - * Gets Metadata about the given GCS object, interpreting it as the specified PathType. - * - * @param itemId The identifier of the GCS object. - * @param pathType The type of path (e.g., file or directory). - * @return Metadata about the given path item. - */ - GcsItemInfo getFileInfo(GcsItemId itemId, PathType pathType) throws IOException; - - /** - * Lists the statuses of the files/directories in the given path. - * - * @param path The path we want to list. - * @return A list of GcsFileInfo. - */ - java.util.List listStatus(URI path) throws IOException; - - /** - * Lists the statuses of the files/directories in the given path. - * - * @param itemId The identifier of the given path. - * @return A list of GcsFileInfo. - */ - java.util.List listStatus(GcsItemId itemId) throws IOException; - - /** - * Creates the directory named by the given identifier, including any necessary but nonexistent - * parent directories. - * - * @param id The identifier for the directory to create. - */ - void mkdirs(GcsItemId id) throws IOException; - - /** - * Deletes the item denoted by the given identifier. If recursive is true and the item is a - * directory, all contents will be deleted. - * - * @param id The identifier of the item to delete. - * @param recursive Whether to recursively delete contents if the item is a directory. - */ - void delete(GcsItemId id, boolean recursive) throws IOException; - - /** - * Renames the item from the source identifier to the destination identifier. - * - * @param src The current identifier of the item. - * @param dst The new identifier for the item. - */ - void rename(GcsItemId src, GcsItemId dst) throws IOException; - - /** - * Retrieves the value of an extended attribute (custom metadata) for the given item. - * - * @param id The identifier of the item. - * @param name The name of the extended attribute. - * @return The byte array value of the extended attribute, or null if it does not exist. - */ - byte[] getXAttr(GcsItemId id, String name) throws IOException; - - /** - * Sets an extended attribute (custom metadata) for the given item. - * - * @param id The identifier of the item. - * @param name The name of the extended attribute. - * @param value The byte array value to set for the attribute. - */ - void setXAttr(GcsItemId id, String name, byte[] value) throws IOException; - - /** - * Retrieves all extended attributes (custom metadata) for the given item. - * - * @param id The identifier of the item. - * @return A map of extended attribute names to their byte array values. - */ - Map getXAttrs(GcsItemId id) throws IOException; - /** Retrieve the options that were used to create this GcsFileSystem. */ GcsFileSystemOptions getFileSystemOptions(); diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 373349d20..9cb90e920 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -19,9 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.auth.Credentials; -import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; import com.google.cloud.gcs.analyticscore.common.GcsAnalyticsCoreTelemetryConstants; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -160,67 +157,6 @@ public GcsFileInfo getFileInfo(GcsItemId itemId) throws IOException { .build(); } - @Override - public GcsItemInfo getFileInfo(GcsItemId itemId, PathType pathType) throws IOException { - return resolveStrategy(itemId.getBucketName()).getFileInfo(itemId, pathType); - } - - @Override - public java.util.List listStatus(URI path) throws IOException { - GcsItemId itemId = UriUtil.getItemIdFromString(path.toString()); - return listStatus(itemId); - } - - @Override - public java.util.List listStatus(GcsItemId itemId) throws IOException { - java.util.List itemInfos = - resolveStrategy(itemId.getBucketName()).listStatus(itemId); - return itemInfos.stream() - .map( - info -> - GcsFileInfo.builder() - .setItemInfo(info) - .setUri( - URI.create( - BlobId.of( - info.getItemId().getBucketName(), - info.getItemId().getObjectName().get()) - .toGsUtilUri())) - .setAttributes(Collections.emptyMap()) - .build()) - .collect(java.util.stream.Collectors.toList()); - } - - @Override - public void mkdirs(GcsItemId id) throws IOException { - resolveStrategy(id.getBucketName()).mkdirs(id); - } - - @Override - public void delete(GcsItemId id, boolean recursive) throws IOException { - resolveStrategy(id.getBucketName()).delete(id, recursive); - } - - @Override - public void rename(GcsItemId src, GcsItemId dst) throws IOException { - resolveStrategy(src.getBucketName()).rename(src, dst); - } - - @Override - public byte[] getXAttr(GcsItemId id, String name) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void setXAttr(GcsItemId id, String name, byte[] value) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public java.util.Map getXAttrs(GcsItemId id) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - @Override public GcsFileSystemOptions getFileSystemOptions() { return this.fileSystemOptions; From 1ca0bedee60e70463d310de5a59da40b18cf8d62 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 17 Jun 2026 10:22:43 +0000 Subject: [PATCH 05/28] test: add HNS support validation tests --- .../analyticscore/client/GcsFileSystem.java | 11 +++ .../client/GcsFileSystemImpl.java | 15 ++++ .../client/GcsFileSystemImplTest.java | 68 ++++++++++++++++++- .../client/GcsFileSystemOptionsTest.java | 22 +++++- 4 files changed, 114 insertions(+), 2 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java index 017f33607..4447f9d9a 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java @@ -15,7 +15,10 @@ */ package com.google.cloud.gcs.analyticscore.client; +import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; import com.google.cloud.gcs.analyticscore.common.telemetry.Telemetry; +import com.google.common.annotations.VisibleForTesting; import java.io.FileNotFoundException; import java.io.IOException; import java.net.URI; @@ -70,6 +73,14 @@ VectoredSeekableByteChannel open(GcsFileInfo gcsFileInfo, GcsReadOptions options /** Returns the cache manager used by this file system. */ AnalyticsCacheManager getCacheManager(); + /** Returns the flat namespace strategy used by this file system. */ + @VisibleForTesting + FlatNamespaceStrategyImpl getFlatStrategy(); + + /** Returns the hierarchical namespace strategy used by this file system. */ + @VisibleForTesting + HierarchicalNamespaceStrategyImpl getHnsStrategy(); + /** Close the file system. */ @Override void close(); diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 9cb90e920..a11062e18 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -19,6 +19,9 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.auth.Credentials; +import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; import com.google.cloud.gcs.analyticscore.common.GcsAnalyticsCoreTelemetryConstants; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -177,6 +180,18 @@ public AnalyticsCacheManager getCacheManager() { return cacheManager; } + @VisibleForTesting + @Override + public FlatNamespaceStrategyImpl getFlatStrategy() { + return flatStrategy; + } + + @VisibleForTesting + @Override + public HierarchicalNamespaceStrategyImpl getHnsStrategy() { + return hnsStrategy; + } + @Override public void close() { ExecutorService executorService = executorServiceSupplier.get(); diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 3897c3e9e..39593df33 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -23,6 +23,9 @@ import static org.mockito.Mockito.*; import com.google.cloud.NoCredentials; +import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; +import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; import com.google.cloud.gcs.analyticscore.common.telemetry.CustomTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -88,6 +91,10 @@ void constructor_withCredentials_createsClientWithProvidedCredentials() { assertThat(gcsClientImpl.storage.getOptions().getCredentials()) .isEqualTo(NoCredentials.getInstance()); + assertThat(gcsFileSystem.getTelemetry()).isNotNull(); + assertThat(gcsFileSystem.getCacheManager()).isNotNull(); + assertThat(gcsFileSystem.getFlatStrategy()).isNotNull(); + assertThat(gcsFileSystem.getHnsStrategy()).isNotNull(); } } @@ -104,12 +111,17 @@ void constructor_withFileSystemOptions_createsClientWithDefaultCredentials() { assertThat(gcsFileSystem.getFileSystemOptions()).isSameInstanceAs(fileSystemOptions); assertThat(gcsClient).isNotNull(); assertThat(gcsClient.storage.getOptions().getProjectId()).isEqualTo("test-project-default"); + assertThat(gcsFileSystem.getTelemetry()).isNotNull(); + assertThat(gcsFileSystem.getCacheManager()).isNotNull(); + assertThat(gcsFileSystem.getFlatStrategy()).isNotNull(); + assertThat(gcsFileSystem.getHnsStrategy()).isNotNull(); } } @Test - void constructor_withValidOptions_passesExecutorToClient() { + void constructor_withValidOptions_passesMemorizedExecutorServiceAndTelemetryToGcsClient() { final AtomicReference> capturedSupplier = new AtomicReference<>(); + final AtomicReference capturedTelemetry = new AtomicReference<>(); try (MockedConstruction mockGcsClientConstruction = Mockito.mockConstruction( GcsClientImpl.class, @@ -118,6 +130,9 @@ void constructor_withValidOptions_passesExecutorToClient() { Supplier supplier = (Supplier) context.arguments().get(1); capturedSupplier.set(supplier); + + Telemetry telemetry = (Telemetry) context.arguments().get(2); + capturedTelemetry.set(telemetry); })) { try (GcsFileSystemImpl fs = new GcsFileSystemImpl(TEST_GCS_FILESYSTEM_OPTIONS)) { @@ -128,6 +143,12 @@ void constructor_withValidOptions_passesExecutorToClient() { assertThat(capturedSupplier.get()).isNotNull(); assertThat(capturedSupplier.get().get()).isNotNull(); assertThat(executorService1).isEqualTo(executorService2); + + assertThat(capturedTelemetry.get()).isNotNull(); + assertThat(capturedTelemetry.get()).isSameInstanceAs(fs.getTelemetry()); + + assertThat(fs.getFlatStrategy()).isNotNull(); + assertThat(fs.getHnsStrategy()).isNotNull(); } } } @@ -574,6 +595,51 @@ void create_nullWriteOptions_delegatesToClientWithNullOptions() throws IOExcepti assertThat(resultChannel).isSameInstanceAs(mockChannel); } + @Test + void resolveStrategy_hnsEnabledInCapabilitiesAndApiEnabled_returnsHnsStrategy() + throws IOException { + GcsFileSystemOptions options = + GcsFileSystemOptions.builder() + .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) + .setHnsApiEnabled(true) + .build(); + try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { + when(mockClient.getBucketCapabilities(TEST_BUCKET)).thenReturn(new BucketCapabilities(true)); + NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); + } + } + + @Test + void resolveStrategy_hnsEnabledInCapabilitiesButApiDisabled_returnsFlatStrategy() + throws IOException { + GcsFileSystemOptions options = + GcsFileSystemOptions.builder() + .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) + .setHnsApiEnabled(false) + .build(); + try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { + when(mockClient.getBucketCapabilities(TEST_BUCKET)).thenReturn(new BucketCapabilities(true)); + NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); + } + } + + @Test + void resolveStrategy_hnsDisabledInCapabilitiesAndApiEnabled_returnsFlatStrategy() + throws IOException { + GcsFileSystemOptions options = + GcsFileSystemOptions.builder() + .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) + .setHnsApiEnabled(true) + .build(); + try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { + when(mockClient.getBucketCapabilities(TEST_BUCKET)).thenReturn(new BucketCapabilities(false)); + NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); + } + } + @SuppressWarnings("unchecked") private List getRegisteredTelemetryListeners(Telemetry telemetry) { try { diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java index 0b4a3ff06..9cb8a323c 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java @@ -32,13 +32,15 @@ void createFromOptions_withValidProperties_shouldCreateCorrectOptions() { ImmutableMap.of( "fs.gs.project-id", "test-project", "fs.gs.client.type", "GRPC_CLIENT", - "fs.gs.analytics-core.read.thread.count", "32"); + "fs.gs.analytics-core.read.thread.count", "32", + "fs.gs.analytics-core.hns.api.enable", "true"); GcsFileSystemOptions options = GcsFileSystemOptions.createFromOptions(properties, "fs.gs."); assertThat(options.getGcsClientOptions().getProjectId().get()).isEqualTo("test-project"); assertThat(options.getClientType()).isEqualTo(GcsFileSystemOptions.ClientType.GRPC_CLIENT); assertThat(options.getReadThreadCount()).isEqualTo(32); + assertThat(options.isHnsApiEnabled()).isTrue(); } @Test @@ -62,4 +64,22 @@ void createFromOptions_cacheProperties_createsCorrectOptions() { assertThat(cacheOptions.isSmallObjectCacheEnabled()).isTrue(); assertThat(cacheOptions.getSmallObjectCacheMaxSizeBytes()).isEqualTo(200 * MB); } + + @Test + void createFromOptions_withDefaultProperties_shouldCreateCorrectOptions() { + ImmutableMap properties = ImmutableMap.of(); + + GcsFileSystemOptions options = GcsFileSystemOptions.createFromOptions(properties, "fs.gs."); + + assertThat(options.getGcsClientOptions().getProjectId().isEmpty()).isTrue(); + assertThat(options.getClientType()).isEqualTo(GcsFileSystemOptions.ClientType.HTTP_CLIENT); + assertThat(options.getReadThreadCount()).isEqualTo(16); + assertThat(options.isHnsApiEnabled()).isFalse(); + + GcsCacheOptions cacheOptions = options.getGcsCacheOptions(); + assertThat(cacheOptions.isFooterCacheEnabled()).isFalse(); + assertThat(cacheOptions.getFooterCacheMaxSizeBytes()).isEqualTo(500 * MB); + assertThat(cacheOptions.isSmallObjectCacheEnabled()).isTrue(); + assertThat(cacheOptions.getSmallObjectCacheMaxSizeBytes()).isEqualTo(200 * MB); + } } From b4f8daa64775a283456b7533745a16efdc71b267 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 17 Jun 2026 11:00:49 +0000 Subject: [PATCH 06/28] refactor: encapsulate namespace strategy accessors --- .../cloud/gcs/analyticscore/client/GcsFileSystem.java | 11 ----------- .../gcs/analyticscore/client/GcsFileSystemImpl.java | 8 +++----- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java index 4447f9d9a..017f33607 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java @@ -15,10 +15,7 @@ */ package com.google.cloud.gcs.analyticscore.client; -import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; import com.google.cloud.gcs.analyticscore.common.telemetry.Telemetry; -import com.google.common.annotations.VisibleForTesting; import java.io.FileNotFoundException; import java.io.IOException; import java.net.URI; @@ -73,14 +70,6 @@ VectoredSeekableByteChannel open(GcsFileInfo gcsFileInfo, GcsReadOptions options /** Returns the cache manager used by this file system. */ AnalyticsCacheManager getCacheManager(); - /** Returns the flat namespace strategy used by this file system. */ - @VisibleForTesting - FlatNamespaceStrategyImpl getFlatStrategy(); - - /** Returns the hierarchical namespace strategy used by this file system. */ - @VisibleForTesting - HierarchicalNamespaceStrategyImpl getHnsStrategy(); - /** Close the file system. */ @Override void close(); diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index a11062e18..c5bcdb801 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -113,7 +113,7 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.cacheManager = cacheManager; } - public NamespaceStrategy resolveStrategy(String bucketName) throws IOException { + NamespaceStrategy resolveStrategy(String bucketName) throws IOException { BucketCapabilities capabilities = cacheManager.getBucketCapabilities(bucketName, gcsClient::getBucketCapabilities); @@ -181,14 +181,12 @@ public AnalyticsCacheManager getCacheManager() { } @VisibleForTesting - @Override - public FlatNamespaceStrategyImpl getFlatStrategy() { + FlatNamespaceStrategyImpl getFlatStrategy() { return flatStrategy; } @VisibleForTesting - @Override - public HierarchicalNamespaceStrategyImpl getHnsStrategy() { + HierarchicalNamespaceStrategyImpl getHnsStrategy() { return hnsStrategy; } From d7ccf9cebdbebb31a93e01e4cfef3c61fbe569ba Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 17 Jun 2026 11:13:16 +0000 Subject: [PATCH 07/28] refactor: migrate BucketCapabilities to an AutoValue class --- .../gcs/analyticscore/client/GcsFileSystemImplTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 39593df33..9da4909cb 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -604,7 +604,8 @@ void resolveStrategy_hnsEnabledInCapabilitiesAndApiEnabled_returnsHnsStrategy() .setHnsApiEnabled(true) .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketCapabilities(TEST_BUCKET)).thenReturn(new BucketCapabilities(true)); + when(mockClient.getBucketCapabilities(TEST_BUCKET)) + .thenReturn(BucketCapabilities.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); } @@ -619,7 +620,8 @@ void resolveStrategy_hnsEnabledInCapabilitiesButApiDisabled_returnsFlatStrategy( .setHnsApiEnabled(false) .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketCapabilities(TEST_BUCKET)).thenReturn(new BucketCapabilities(true)); + when(mockClient.getBucketCapabilities(TEST_BUCKET)) + .thenReturn(BucketCapabilities.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } @@ -634,7 +636,8 @@ void resolveStrategy_hnsDisabledInCapabilitiesAndApiEnabled_returnsFlatStrategy( .setHnsApiEnabled(true) .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketCapabilities(TEST_BUCKET)).thenReturn(new BucketCapabilities(false)); + when(mockClient.getBucketCapabilities(TEST_BUCKET)) + .thenReturn(BucketCapabilities.create(false)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } From 7890c16543a3ee6dc796f9bf682fa647f5968cf6 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 30 Jun 2026 15:38:21 +0000 Subject: [PATCH 08/28] refactor: replace BucketCapabilities with BucketProperties --- .../client/GcsFileSystemImpl.java | 6 +++--- .../client/GcsFileSystemImplTest.java | 18 +++++++++--------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index c5bcdb801..af30d97d4 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -114,10 +114,10 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste } NamespaceStrategy resolveStrategy(String bucketName) throws IOException { - BucketCapabilities capabilities = - cacheManager.getBucketCapabilities(bucketName, gcsClient::getBucketCapabilities); + BucketProperties properties = + cacheManager.getBucketProperties(bucketName, gcsClient::getBucketProperties); - if (capabilities.isHnsEnabled() && fileSystemOptions.isHnsApiEnabled()) { + if (properties.isHnsEnabled() && fileSystemOptions.isHnsApiEnabled()) { return hnsStrategy; } return flatStrategy; diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 9da4909cb..bd67f0bb9 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -596,7 +596,7 @@ void create_nullWriteOptions_delegatesToClientWithNullOptions() throws IOExcepti } @Test - void resolveStrategy_hnsEnabledInCapabilitiesAndApiEnabled_returnsHnsStrategy() + void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() @@ -604,15 +604,15 @@ void resolveStrategy_hnsEnabledInCapabilitiesAndApiEnabled_returnsHnsStrategy() .setHnsApiEnabled(true) .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketCapabilities(TEST_BUCKET)) - .thenReturn(BucketCapabilities.create(true)); + when(mockClient.getBucketProperties(TEST_BUCKET)) + .thenReturn(BucketProperties.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); } } @Test - void resolveStrategy_hnsEnabledInCapabilitiesButApiDisabled_returnsFlatStrategy() + void resolveStrategy_hnsEnabledInPropertiesButApiDisabled_returnsFlatStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() @@ -620,15 +620,15 @@ void resolveStrategy_hnsEnabledInCapabilitiesButApiDisabled_returnsFlatStrategy( .setHnsApiEnabled(false) .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketCapabilities(TEST_BUCKET)) - .thenReturn(BucketCapabilities.create(true)); + when(mockClient.getBucketProperties(TEST_BUCKET)) + .thenReturn(BucketProperties.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } } @Test - void resolveStrategy_hnsDisabledInCapabilitiesAndApiEnabled_returnsFlatStrategy() + void resolveStrategy_hnsDisabledInPropertiesAndApiEnabled_returnsFlatStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() @@ -636,8 +636,8 @@ void resolveStrategy_hnsDisabledInCapabilitiesAndApiEnabled_returnsFlatStrategy( .setHnsApiEnabled(true) .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketCapabilities(TEST_BUCKET)) - .thenReturn(BucketCapabilities.create(false)); + when(mockClient.getBucketProperties(TEST_BUCKET)) + .thenReturn(BucketProperties.create(false)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } From 58c3cf1848b412c8f099d3cb84bc1eae54244ff3 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 7 Jul 2026 08:21:02 +0000 Subject: [PATCH 09/28] refactor: Move NamespaceStrategy to client package and functionally inject BucketPropertiesLoader to GcsFileSystemImpl --- .../client/FlatNamespaceStrategyImpl.java | 19 +++++++ .../client/GcsFileSystemImpl.java | 40 +++++++++------ .../HierarchicalNamespaceStrategyImpl.java | 19 +++++++ .../client/NamespaceStrategy.java | 19 +++++++ .../namespace/FlatNamespaceStrategyImpl.java | 49 ------------------- .../HierarchicalNamespaceStrategyImpl.java | 49 ------------------- .../client/namespace/NamespaceStrategy.java | 34 ------------- .../client/GcsFileSystemImplTest.java | 35 +++++++------ .../client/GcsFileSystemOptionsTest.java | 4 +- .../client/FakeGcsClientImpl.java | 6 +++ .../client/FakeGcsFileSystemImpl.java | 13 +++-- 11 files changed, 119 insertions(+), 168 deletions(-) create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java delete mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java delete mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java delete mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java new file mode 100644 index 000000000..b1d1b2f29 --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java @@ -0,0 +1,19 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client; + +class FlatNamespaceStrategyImpl implements NamespaceStrategy {} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index af30d97d4..16eb66996 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -19,9 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.auth.Credentials; -import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; import com.google.cloud.gcs.analyticscore.common.GcsAnalyticsCoreTelemetryConstants; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -53,6 +50,7 @@ public class GcsFileSystemImpl implements GcsFileSystem { private final Telemetry telemetry; private final AnalyticsCacheManager cacheManager; + private final AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider; private final FlatNamespaceStrategyImpl flatStrategy = new FlatNamespaceStrategyImpl(); private final HierarchicalNamespaceStrategyImpl hnsStrategy = @@ -63,14 +61,16 @@ public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()); this.cacheManager = new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()); + GcsClientImpl clientImpl = + new GcsClientImpl( + fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry); this.gcsClient = telemetry.measure( GcsAnalyticsCoreTelemetryConstants.Operation.GCS_CLIENT_CREATE.name(), GcsAnalyticsCoreTelemetryConstants.Metric.GCS_CLIENT_CREATE_DURATION, Collections.emptyMap(), - recorder -> - new GcsClientImpl( - fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry)); + recorder -> clientImpl); + this.bucketPropertiesProvider = clientImpl::getBucketProperties; } public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSystemOptions) { @@ -78,26 +78,32 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()); this.cacheManager = new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()); + GcsClientImpl clientImpl = + new GcsClientImpl( + credentials, + fileSystemOptions.getGcsClientOptions(), + executorServiceSupplier, + telemetry); this.gcsClient = telemetry.measure( GcsAnalyticsCoreTelemetryConstants.Operation.GCS_CLIENT_CREATE.name(), GcsAnalyticsCoreTelemetryConstants.Metric.GCS_CLIENT_CREATE_DURATION, Collections.emptyMap(), - recorder -> - new GcsClientImpl( - credentials, - fileSystemOptions.getGcsClientOptions(), - executorServiceSupplier, - telemetry)); + recorder -> clientImpl); + this.bucketPropertiesProvider = clientImpl::getBucketProperties; } @VisibleForTesting - GcsFileSystemImpl(GcsClient gcsClient, GcsFileSystemOptions fileSystemOptions) { + GcsFileSystemImpl( + GcsClient gcsClient, + AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider, + GcsFileSystemOptions fileSystemOptions) { this( gcsClient, fileSystemOptions, createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()), - new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions())); + new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()), + bucketPropertiesProvider); } @VisibleForTesting @@ -105,17 +111,19 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste GcsClient gcsClient, GcsFileSystemOptions fileSystemOptions, Telemetry telemetry, - AnalyticsCacheManager cacheManager) { + AnalyticsCacheManager cacheManager, + AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider) { this.gcsClient = gcsClient; this.fileSystemOptions = fileSystemOptions; this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = telemetry; this.cacheManager = cacheManager; + this.bucketPropertiesProvider = bucketPropertiesProvider; } NamespaceStrategy resolveStrategy(String bucketName) throws IOException { BucketProperties properties = - cacheManager.getBucketProperties(bucketName, gcsClient::getBucketProperties); + cacheManager.getBucketProperties(bucketName, bucketPropertiesProvider); if (properties.isHnsEnabled() && fileSystemOptions.isHnsApiEnabled()) { return hnsStrategy; diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java new file mode 100644 index 000000000..a38a886f9 --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java @@ -0,0 +1,19 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client; + +class HierarchicalNamespaceStrategyImpl implements NamespaceStrategy {} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java new file mode 100644 index 000000000..212110868 --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java @@ -0,0 +1,19 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client; + +interface NamespaceStrategy {} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java deleted file mode 100644 index 8f729aa3d..000000000 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/FlatNamespaceStrategyImpl.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * Copyright 2026 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.cloud.gcs.analyticscore.client.namespace; - -import com.google.cloud.gcs.analyticscore.client.GcsItemId; -import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; -import com.google.cloud.gcs.analyticscore.client.PathType; -import java.io.IOException; - -public class FlatNamespaceStrategyImpl implements NamespaceStrategy { - @Override - public GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void mkdirs(GcsItemId id) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void delete(GcsItemId id, boolean recursive) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void rename(GcsItemId src, GcsItemId dst) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public java.util.List listStatus(GcsItemId id) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } -} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java deleted file mode 100644 index bbcbebcc3..000000000 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/HierarchicalNamespaceStrategyImpl.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * Copyright 2026 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.cloud.gcs.analyticscore.client.namespace; - -import com.google.cloud.gcs.analyticscore.client.GcsItemId; -import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; -import com.google.cloud.gcs.analyticscore.client.PathType; -import java.io.IOException; - -public class HierarchicalNamespaceStrategyImpl implements NamespaceStrategy { - @Override - public GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void mkdirs(GcsItemId id) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void delete(GcsItemId id, boolean recursive) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public void rename(GcsItemId src, GcsItemId dst) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } - - @Override - public java.util.List listStatus(GcsItemId id) throws IOException { - throw new UnsupportedOperationException("Not implemented yet"); - } -} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java deleted file mode 100644 index 009357057..000000000 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/namespace/NamespaceStrategy.java +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright 2026 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.cloud.gcs.analyticscore.client.namespace; - -import com.google.cloud.gcs.analyticscore.client.GcsItemId; -import com.google.cloud.gcs.analyticscore.client.GcsItemInfo; -import com.google.cloud.gcs.analyticscore.client.PathType; -import java.io.IOException; - -public interface NamespaceStrategy { - GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException; - - void mkdirs(GcsItemId id) throws IOException; - - void delete(GcsItemId id, boolean recursive) throws IOException; - - void rename(GcsItemId src, GcsItemId dst) throws IOException; - - java.util.List listStatus(GcsItemId id) throws IOException; -} diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index bd67f0bb9..6805e2210 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -23,9 +23,6 @@ import static org.mockito.Mockito.*; import com.google.cloud.NoCredentials; -import com.google.cloud.gcs.analyticscore.client.namespace.FlatNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.HierarchicalNamespaceStrategyImpl; -import com.google.cloud.gcs.analyticscore.client.namespace.NamespaceStrategy; import com.google.cloud.gcs.analyticscore.common.telemetry.CustomTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryOptions; import com.google.cloud.gcs.analyticscore.common.telemetry.LoggingTelemetryReporter; @@ -69,11 +66,14 @@ class GcsFileSystemImplTest { GcsFileSystemOptions.builder().setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS).build(); @Mock private GcsClient mockClient; + @Mock private AnalyticsCacheManager.BucketPropertiesLoader mockBucketPropertiesProvider; private GcsFileSystem gcsFileSystem; @BeforeEach void setUp() { - gcsFileSystem = new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS); + gcsFileSystem = + new GcsFileSystemImpl( + mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS); } @AfterEach @@ -337,7 +337,8 @@ void close_whenTerminationSucceeds_shutsDownGracefully() throws InterruptedExcep ExecutorService mockExecutorService = mock(ExecutorService.class); when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(true); GcsFileSystemImpl fileSystemWithMockExecutor = - new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { + new GcsFileSystemImpl( + mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS) { @Override Supplier initializeExecutionServiceSupplier() { return () -> mockExecutorService; @@ -358,7 +359,8 @@ void close_whenTerminationTimesOut_shutsDownNow() throws InterruptedException { ExecutorService mockExecutorService = mock(ExecutorService.class); when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(false); GcsFileSystemImpl fileSystemWithMockExecutor = - new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { + new GcsFileSystemImpl( + mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS) { @Override Supplier initializeExecutionServiceSupplier() { return () -> mockExecutorService; @@ -380,7 +382,8 @@ void close_whenInterrupted_reInterruptsThreadAndShutsDownNow() throws Interrupte when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) .thenThrow(new InterruptedException()); GcsFileSystemImpl fileSystemWithMockExecutor = - new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { + new GcsFileSystemImpl( + mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS) { @Override Supplier initializeExecutionServiceSupplier() { return () -> mockExecutorService; @@ -596,15 +599,15 @@ void create_nullWriteOptions_delegatesToClientWithNullOptions() throws IOExcepti } @Test - void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() - throws IOException { + void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(true) .build(); - try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketProperties(TEST_BUCKET)) + try (GcsFileSystemImpl fs = + new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { + when(mockBucketPropertiesProvider.load(TEST_BUCKET)) .thenReturn(BucketProperties.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); @@ -619,8 +622,9 @@ void resolveStrategy_hnsEnabledInPropertiesButApiDisabled_returnsFlatStrategy() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(false) .build(); - try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketProperties(TEST_BUCKET)) + try (GcsFileSystemImpl fs = + new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { + when(mockBucketPropertiesProvider.load(TEST_BUCKET)) .thenReturn(BucketProperties.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); @@ -635,8 +639,9 @@ void resolveStrategy_hnsDisabledInPropertiesAndApiEnabled_returnsFlatStrategy() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(true) .build(); - try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.getBucketProperties(TEST_BUCKET)) + try (GcsFileSystemImpl fs = + new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { + when(mockBucketPropertiesProvider.load(TEST_BUCKET)) .thenReturn(BucketProperties.create(false)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java index 9cb8a323c..1fbf99852 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java @@ -78,8 +78,8 @@ void createFromOptions_withDefaultProperties_shouldCreateCorrectOptions() { GcsCacheOptions cacheOptions = options.getGcsCacheOptions(); assertThat(cacheOptions.isFooterCacheEnabled()).isFalse(); - assertThat(cacheOptions.getFooterCacheMaxSizeBytes()).isEqualTo(500 * MB); - assertThat(cacheOptions.isSmallObjectCacheEnabled()).isTrue(); + assertThat(cacheOptions.getFooterCacheMaxSizeBytes()).isEqualTo(100 * MB); + assertThat(cacheOptions.isSmallObjectCacheEnabled()).isFalse(); assertThat(cacheOptions.getSmallObjectCacheMaxSizeBytes()).isEqualTo(200 * MB); } } diff --git a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsClientImpl.java b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsClientImpl.java index 81c175217..69efabf24 100644 --- a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsClientImpl.java +++ b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsClientImpl.java @@ -51,6 +51,12 @@ protected Storage createStorage(Optional credentials) { return storage; } + @Override + BucketProperties getBucketProperties(String bucketName) { + // FakeStorageRpc does not support bucket operations + return BucketProperties.create(false); + } + @Override public VectoredSeekableByteChannel openReadChannel( GcsItemInfo itemInfo, GcsReadOptions readOptions) throws IOException { diff --git a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java index a1f081380..727fe2dc5 100644 --- a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java +++ b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java @@ -29,14 +29,21 @@ public FakeGcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { } private FakeGcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions, Telemetry telemetry) { + this(fileSystemOptions, telemetry, initializeGcsClient(fileSystemOptions, telemetry)); + } + + private FakeGcsFileSystemImpl( + GcsFileSystemOptions fileSystemOptions, Telemetry telemetry, FakeGcsClientImpl fakeClient) { super( - initializeGcsClient(fileSystemOptions, telemetry), + fakeClient, fileSystemOptions, telemetry, - new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions())); + new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()), + fakeClient::getBucketProperties); } - private static GcsClient initializeGcsClient(GcsFileSystemOptions options, Telemetry telemetry) { + private static FakeGcsClientImpl initializeGcsClient( + GcsFileSystemOptions options, Telemetry telemetry) { Supplier executorServiceSupplier = Suppliers.ofInstance(Executors.newCachedThreadPool()); return new FakeGcsClientImpl(options.getGcsClientOptions(), executorServiceSupplier, telemetry); From c5316b68cf8ea73414f9dc0f299a26e98577d342 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 7 Jul 2026 08:40:41 +0000 Subject: [PATCH 10/28] doc: add HNS API configuration in doc and annotate resolveStrategy with VisibleForTesting --- CONFIGURATION.md | 1 + .../google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java | 1 + 2 files changed, 2 insertions(+) diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 8a7cd52cb..0121670b3 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -18,6 +18,7 @@ These properties govern the core connections, identity, and access parameters be | `project-id` | The Google Cloud project ID for the GCS client. | - | | `user-project` | Project ID whose Google Cloud Project's billing account should be charged for the operation being executed. | - | | `decryption-key` | Decryption key for the object. | - | +| `analytics-core.hns.api.enable` | Controls whether the Hierarchical Namespace (HNS) API is enabled for operations. | `false` | ### Caching and Prefetching diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 16eb66996..109df996e 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -121,6 +121,7 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.bucketPropertiesProvider = bucketPropertiesProvider; } + @VisibleForTesting NamespaceStrategy resolveStrategy(String bucketName) throws IOException { BucketProperties properties = cacheManager.getBucketProperties(bucketName, bucketPropertiesProvider); From 8903dd00cec7419e5e792de9f0b67c451ec1ec6a Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 14 Jul 2026 11:06:32 +0000 Subject: [PATCH 11/28] refactor: inject GcsClient into namespace strategies and improve code readability --- CONFIGURATION.md | 8 +++- .../client/FlatNamespaceStrategyImpl.java | 8 +++- .../analyticscore/client/GcsFileSystem.java | 1 - .../client/GcsFileSystemImpl.java | 37 +++++++++++-------- .../HierarchicalNamespaceStrategyImpl.java | 8 +++- .../client/GcsFileSystemImplTest.java | 6 +++ 6 files changed, 48 insertions(+), 20 deletions(-) diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 0121670b3..ad61357fe 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -18,7 +18,6 @@ These properties govern the core connections, identity, and access parameters be | `project-id` | The Google Cloud project ID for the GCS client. | - | | `user-project` | Project ID whose Google Cloud Project's billing account should be charged for the operation being executed. | - | | `decryption-key` | Decryption key for the object. | - | -| `analytics-core.hns.api.enable` | Controls whether the Hierarchical Namespace (HNS) API is enabled for operations. | `false` | ### Caching and Prefetching @@ -50,6 +49,13 @@ These parameters fine-tune the low-level data streaming behavior. They allow you | `analytics-core.adaptive-read.sequential-read-threshold` | Threshold for number of sequential reads to switch to sequential mode. | `3` | | `analytics-core.random-read.min-request-size` | Minimum request size for random reads. If the requested read size is smaller, it reads up to this size. | `131072` (128 KB) | +### Metadata and Directory Operations + +These properties configure how the library handles file system metadata operations like directory creation, listing or file status retrieval. + +| Property | Description | Default Value | +| :--- | :--- | :--- | +| `analytics-core.hns.api.enable` | Controls whether the Hierarchical Namespace (HNS) API is enabled for operations. | `false` | ### Telemetry and Monitoring diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java index b1d1b2f29..fe5e203cc 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java @@ -16,4 +16,10 @@ package com.google.cloud.gcs.analyticscore.client; -class FlatNamespaceStrategyImpl implements NamespaceStrategy {} +final class FlatNamespaceStrategyImpl implements NamespaceStrategy { + private final GcsClient gcsClient; + + FlatNamespaceStrategyImpl(GcsClient gcsClient) { + this.gcsClient = gcsClient; + } +} diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java index 017f33607..4a3411e21 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystem.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.net.URI; import java.nio.channels.WritableByteChannel; -import java.util.Map; public interface GcsFileSystem extends AutoCloseable { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 109df996e..48551131b 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -52,25 +52,25 @@ public class GcsFileSystemImpl implements GcsFileSystem { private final AnalyticsCacheManager cacheManager; private final AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider; - private final FlatNamespaceStrategyImpl flatStrategy = new FlatNamespaceStrategyImpl(); - private final HierarchicalNamespaceStrategyImpl hnsStrategy = - new HierarchicalNamespaceStrategyImpl(); + private final FlatNamespaceStrategyImpl flatStrategy; + private final HierarchicalNamespaceStrategyImpl hnsStrategy; public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { this.fileSystemOptions = fileSystemOptions; this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()); this.cacheManager = new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()); - GcsClientImpl clientImpl = - new GcsClientImpl( - fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry); this.gcsClient = telemetry.measure( GcsAnalyticsCoreTelemetryConstants.Operation.GCS_CLIENT_CREATE.name(), GcsAnalyticsCoreTelemetryConstants.Metric.GCS_CLIENT_CREATE_DURATION, Collections.emptyMap(), - recorder -> clientImpl); - this.bucketPropertiesProvider = clientImpl::getBucketProperties; + recorder -> + new GcsClientImpl( + fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry)); + this.bucketPropertiesProvider = ((GcsClientImpl) this.gcsClient)::getBucketProperties; + this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); + this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSystemOptions) { @@ -78,19 +78,20 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()); this.cacheManager = new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()); - GcsClientImpl clientImpl = - new GcsClientImpl( - credentials, - fileSystemOptions.getGcsClientOptions(), - executorServiceSupplier, - telemetry); this.gcsClient = telemetry.measure( GcsAnalyticsCoreTelemetryConstants.Operation.GCS_CLIENT_CREATE.name(), GcsAnalyticsCoreTelemetryConstants.Metric.GCS_CLIENT_CREATE_DURATION, Collections.emptyMap(), - recorder -> clientImpl); - this.bucketPropertiesProvider = clientImpl::getBucketProperties; + recorder -> + new GcsClientImpl( + credentials, + fileSystemOptions.getGcsClientOptions(), + executorServiceSupplier, + telemetry)); + this.bucketPropertiesProvider = ((GcsClientImpl) this.gcsClient)::getBucketProperties; + this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); + this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @VisibleForTesting @@ -119,10 +120,14 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.telemetry = telemetry; this.cacheManager = cacheManager; this.bucketPropertiesProvider = bucketPropertiesProvider; + this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); + this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @VisibleForTesting NamespaceStrategy resolveStrategy(String bucketName) throws IOException { + checkNotNull(bucketName, "bucketName cannot be null"); + checkNotNull(bucketPropertiesProvider, "bucketPropertiesProvider cannot be null"); BucketProperties properties = cacheManager.getBucketProperties(bucketName, bucketPropertiesProvider); diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java index a38a886f9..b05e86520 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/HierarchicalNamespaceStrategyImpl.java @@ -16,4 +16,10 @@ package com.google.cloud.gcs.analyticscore.client; -class HierarchicalNamespaceStrategyImpl implements NamespaceStrategy {} +final class HierarchicalNamespaceStrategyImpl implements NamespaceStrategy { + private final GcsClient gcsClient; + + HierarchicalNamespaceStrategyImpl(GcsClient gcsClient) { + this.gcsClient = gcsClient; + } +} diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 6805e2210..b4503eb80 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -609,7 +609,9 @@ void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() th new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { when(mockBucketPropertiesProvider.load(TEST_BUCKET)) .thenReturn(BucketProperties.create(true)); + NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); } } @@ -626,7 +628,9 @@ void resolveStrategy_hnsEnabledInPropertiesButApiDisabled_returnsFlatStrategy() new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { when(mockBucketPropertiesProvider.load(TEST_BUCKET)) .thenReturn(BucketProperties.create(true)); + NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } } @@ -643,7 +647,9 @@ void resolveStrategy_hnsDisabledInPropertiesAndApiEnabled_returnsFlatStrategy() new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { when(mockBucketPropertiesProvider.load(TEST_BUCKET)) .thenReturn(BucketProperties.create(false)); + NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } } From 55a147a568dbe29308d52d9f8c9f46d73cad4de5 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 14 Jul 2026 11:42:57 +0000 Subject: [PATCH 12/28] feat: optimize namespace strategy resolution and add javadoc for namespaceStrategy interface --- .../gcs/analyticscore/client/GcsFileSystemImpl.java | 6 +++++- .../gcs/analyticscore/client/NamespaceStrategy.java | 13 +++++++++++++ .../analyticscore/client/GcsFileSystemImplTest.java | 6 ++---- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 48551131b..31afd7d1c 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -128,10 +128,14 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste NamespaceStrategy resolveStrategy(String bucketName) throws IOException { checkNotNull(bucketName, "bucketName cannot be null"); checkNotNull(bucketPropertiesProvider, "bucketPropertiesProvider cannot be null"); + if (!fileSystemOptions.isHnsApiEnabled()) { + return flatStrategy; + } + BucketProperties properties = cacheManager.getBucketProperties(bucketName, bucketPropertiesProvider); - if (properties.isHnsEnabled() && fileSystemOptions.isHnsApiEnabled()) { + if (properties.isHnsEnabled()) { return hnsStrategy; } return flatStrategy; diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java index 212110868..959bf7d3e 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java @@ -16,4 +16,17 @@ package com.google.cloud.gcs.analyticscore.client; +/** + * Strategy interface for namespace operations. + * + *

Methods for directory operations will be added in follow-up PRs. These methods will include: + * + *

    + *
  • {@code GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException;} + *
  • {@code void mkdirs(GcsItemId id) throws IOException;} + *
  • {@code void delete(GcsItemId id, boolean recursive) throws IOException;} + *
  • {@code void rename(GcsItemId src, GcsItemId dst) throws IOException;} + *
  • {@code java.util.List listStatus(GcsItemId id) throws IOException;} + *
+ */ interface NamespaceStrategy {} diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index b4503eb80..b4cc5558b 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -617,8 +617,7 @@ void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() th } @Test - void resolveStrategy_hnsEnabledInPropertiesButApiDisabled_returnsFlatStrategy() - throws IOException { + void resolveStrategy_hnsApiDisabled_returnsFlatStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) @@ -626,12 +625,11 @@ void resolveStrategy_hnsEnabledInPropertiesButApiDisabled_returnsFlatStrategy() .build(); try (GcsFileSystemImpl fs = new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { - when(mockBucketPropertiesProvider.load(TEST_BUCKET)) - .thenReturn(BucketProperties.create(true)); NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); + verify(mockBucketPropertiesProvider, never()).load(anyString()); } } From 6a4402455b36ac908df8d0206558d919c53c03ae Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 29 Jul 2026 08:29:24 +0000 Subject: [PATCH 13/28] refactor: remove BucketPropertiesLoader dependency in favor of GcsClient.isHnsBucket method --- .../gcs/analyticscore/client/GcsClient.java | 2 + .../analyticscore/client/GcsClientImpl.java | 5 +++ .../client/GcsFileSystemImpl.java | 19 +++------ .../client/GcsFileSystemImplTest.java | 39 +++++++------------ .../client/FakeGcsFileSystemImpl.java | 3 +- 5 files changed, 27 insertions(+), 41 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClient.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClient.java index 47d4d0ab1..1b20984eb 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClient.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClient.java @@ -43,6 +43,8 @@ WritableByteChannel createWriteChannel(GcsItemId itemId, GcsWriteOptions options /** Fetches object metadata. */ GcsItemInfo getGcsItemInfo(GcsItemId itemId) throws IOException; + boolean isHnsBucket(String bucketName) throws IOException; + /** Close the client. */ void close(); } diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClientImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClientImpl.java index b9ee619cf..07e7461f0 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClientImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsClientImpl.java @@ -171,6 +171,11 @@ BucketProperties getBucketProperties(String bucketName) throws IOException { } } + @Override + public boolean isHnsBucket(String bucketName) throws IOException { + return getBucketProperties(bucketName).isHnsEnabled(); + } + @Override public void close() { try { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 31afd7d1c..e3d54eb34 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -50,7 +50,6 @@ public class GcsFileSystemImpl implements GcsFileSystem { private final Telemetry telemetry; private final AnalyticsCacheManager cacheManager; - private final AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider; private final FlatNamespaceStrategyImpl flatStrategy; private final HierarchicalNamespaceStrategyImpl hnsStrategy; @@ -68,7 +67,6 @@ public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { recorder -> new GcsClientImpl( fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry)); - this.bucketPropertiesProvider = ((GcsClientImpl) this.gcsClient)::getBucketProperties; this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -89,22 +87,17 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry)); - this.bucketPropertiesProvider = ((GcsClientImpl) this.gcsClient)::getBucketProperties; this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @VisibleForTesting - GcsFileSystemImpl( - GcsClient gcsClient, - AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider, - GcsFileSystemOptions fileSystemOptions) { + GcsFileSystemImpl(GcsClient gcsClient, GcsFileSystemOptions fileSystemOptions) { this( gcsClient, fileSystemOptions, createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()), - new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()), - bucketPropertiesProvider); + new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions())); } @VisibleForTesting @@ -112,14 +105,12 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste GcsClient gcsClient, GcsFileSystemOptions fileSystemOptions, Telemetry telemetry, - AnalyticsCacheManager cacheManager, - AnalyticsCacheManager.BucketPropertiesLoader bucketPropertiesProvider) { + AnalyticsCacheManager cacheManager) { this.gcsClient = gcsClient; this.fileSystemOptions = fileSystemOptions; this.executorServiceSupplier = initializeExecutionServiceSupplier(); this.telemetry = telemetry; this.cacheManager = cacheManager; - this.bucketPropertiesProvider = bucketPropertiesProvider; this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -127,13 +118,13 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste @VisibleForTesting NamespaceStrategy resolveStrategy(String bucketName) throws IOException { checkNotNull(bucketName, "bucketName cannot be null"); - checkNotNull(bucketPropertiesProvider, "bucketPropertiesProvider cannot be null"); if (!fileSystemOptions.isHnsApiEnabled()) { return flatStrategy; } BucketProperties properties = - cacheManager.getBucketProperties(bucketName, bucketPropertiesProvider); + cacheManager.getBucketProperties( + bucketName, name -> BucketProperties.create(gcsClient.isHnsBucket(name))); if (properties.isHnsEnabled()) { return hnsStrategy; diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index b4cc5558b..1480bda00 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -66,14 +66,12 @@ class GcsFileSystemImplTest { GcsFileSystemOptions.builder().setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS).build(); @Mock private GcsClient mockClient; - @Mock private AnalyticsCacheManager.BucketPropertiesLoader mockBucketPropertiesProvider; + private GcsFileSystem gcsFileSystem; @BeforeEach void setUp() { - gcsFileSystem = - new GcsFileSystemImpl( - mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS); + gcsFileSystem = new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS); } @AfterEach @@ -337,8 +335,7 @@ void close_whenTerminationSucceeds_shutsDownGracefully() throws InterruptedExcep ExecutorService mockExecutorService = mock(ExecutorService.class); when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(true); GcsFileSystemImpl fileSystemWithMockExecutor = - new GcsFileSystemImpl( - mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS) { + new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override Supplier initializeExecutionServiceSupplier() { return () -> mockExecutorService; @@ -359,8 +356,7 @@ void close_whenTerminationTimesOut_shutsDownNow() throws InterruptedException { ExecutorService mockExecutorService = mock(ExecutorService.class); when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(false); GcsFileSystemImpl fileSystemWithMockExecutor = - new GcsFileSystemImpl( - mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS) { + new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override Supplier initializeExecutionServiceSupplier() { return () -> mockExecutorService; @@ -382,8 +378,7 @@ void close_whenInterrupted_reInterruptsThreadAndShutsDownNow() throws Interrupte when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) .thenThrow(new InterruptedException()); GcsFileSystemImpl fileSystemWithMockExecutor = - new GcsFileSystemImpl( - mockClient, mockBucketPropertiesProvider, TEST_GCS_FILESYSTEM_OPTIONS) { + new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override Supplier initializeExecutionServiceSupplier() { return () -> mockExecutorService; @@ -605,12 +600,10 @@ void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() th .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(true) .build(); - try (GcsFileSystemImpl fs = - new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { - when(mockBucketPropertiesProvider.load(TEST_BUCKET)) - .thenReturn(BucketProperties.create(true)); + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { + when(mockClient.isHnsBucket(TEST_BUCKET)).thenReturn(true); - NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + NamespaceStrategy strategy = gcsFileSystem.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); } @@ -623,13 +616,11 @@ void resolveStrategy_hnsApiDisabled_returnsFlatStrategy() throws IOException { .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(false) .build(); - try (GcsFileSystemImpl fs = - new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { - - NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { + NamespaceStrategy strategy = gcsFileSystem.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); - verify(mockBucketPropertiesProvider, never()).load(anyString()); + verify(mockClient, never()).isHnsBucket(anyString()); } } @@ -641,12 +632,10 @@ void resolveStrategy_hnsDisabledInPropertiesAndApiEnabled_returnsFlatStrategy() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(true) .build(); - try (GcsFileSystemImpl fs = - new GcsFileSystemImpl(mockClient, mockBucketPropertiesProvider, options)) { - when(mockBucketPropertiesProvider.load(TEST_BUCKET)) - .thenReturn(BucketProperties.create(false)); + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { + when(mockClient.isHnsBucket(TEST_BUCKET)).thenReturn(false); - NamespaceStrategy strategy = fs.resolveStrategy(TEST_BUCKET); + NamespaceStrategy strategy = gcsFileSystem.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); } diff --git a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java index 727fe2dc5..bd66091ae 100644 --- a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java +++ b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java @@ -38,8 +38,7 @@ private FakeGcsFileSystemImpl( fakeClient, fileSystemOptions, telemetry, - new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()), - fakeClient::getBucketProperties); + new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions())); } private static FakeGcsClientImpl initializeGcsClient( From 5d37642650e8224d3eee396d8b4a627d123d747a Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 29 Jul 2026 08:40:27 +0000 Subject: [PATCH 14/28] refactor: update NamespaceStrategy interface methods for improved directory and listing operations --- .../gcs/analyticscore/client/NamespaceStrategy.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java index 959bf7d3e..0512a1bf8 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java @@ -23,10 +23,11 @@ * *
    *
  • {@code GcsItemInfo getFileInfo(GcsItemId id, PathType pathType) throws IOException;} - *
  • {@code void mkdirs(GcsItemId id) throws IOException;} - *
  • {@code void delete(GcsItemId id, boolean recursive) throws IOException;} - *
  • {@code void rename(GcsItemId src, GcsItemId dst) throws IOException;} - *
  • {@code java.util.List listStatus(GcsItemId id) throws IOException;} + *
  • {@code void createDirectory(GcsItemId id) throws IOException;} + *
  • {@code boolean isDirectoryEmpty(GcsItemId id) throws IOException;} + *
  • {@code void renameDirectory(GcsItemId src, GcsItemId dst) throws IOException;} + *
  • {@code java.util.List listObjectInfo(GcsItemId id) throws IOException;} + *
  • {@code java.util.List listRecursive(GcsItemId id) throws IOException;} *
*/ interface NamespaceStrategy {} From bf608db88b36396e44d0cc9838e00ff2f3b96442 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 29 Jul 2026 08:52:36 +0000 Subject: [PATCH 15/28] test: add unit tests for GcsClientImpl.isHnsBucket method --- .../client/GcsClientImplTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java index a67e72d44..35c5ca6a2 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java @@ -323,6 +323,18 @@ void getBucketProperties_hnsEnabled_returnsTrue() throws IOException { assertThat(bucketProperties.isHnsEnabled()).isTrue(); } + @Test + void isHnsBucket_hnsEnabled_returnsTrue() throws IOException { + Storage mockStorage = mock(Storage.class); + GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); + Bucket mockBucket = mockBucketWithHns(true); + doReturn(mockBucket).when(mockStorage).get(eq("hns-bucket"), any(BucketGetOption.class)); + + boolean isHns = localGcsClient.isHnsBucket("hns-bucket"); + + assertThat(isHns).isTrue(); + } + @Test void getBucketProperties_hnsDisabled_returnsFalse() throws IOException { Storage mockStorage = mock(Storage.class); @@ -335,6 +347,18 @@ void getBucketProperties_hnsDisabled_returnsFalse() throws IOException { assertThat(bucketProperties.isHnsEnabled()).isFalse(); } + @Test + void isHnsBucket_hnsDisabled_returnsFalse() throws IOException { + Storage mockStorage = mock(Storage.class); + GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); + Bucket mockBucket = mockBucketWithHns(false); + doReturn(mockBucket).when(mockStorage).get(eq("flat-bucket"), any(BucketGetOption.class)); + + boolean isHns = localGcsClient.isHnsBucket("flat-bucket"); + + assertThat(isHns).isFalse(); + } + @Test void getBucketProperties_hnsNull_returnsFalse() throws IOException { Storage mockStorage = mock(Storage.class); From 102fbfda9f79620f40d8457b52f6b4b59b7e7338 Mon Sep 17 00:00:00 2001 From: suni72 Date: Mon, 3 Aug 2026 13:10:07 +0000 Subject: [PATCH 16/28] address reviewer comments: - rename HNS configuration key to hierarchical.namespace.enable - Update test names for clarity - Reverted unrelated changes in FakeGcsFileSystemImpl Updated tests to use constants instead of hardcoded strings --- CONFIGURATION.md | 8 ---- .../client/GcsFileSystemOptions.java | 2 +- .../client/NamespaceStrategy.java | 3 +- .../client/GcsClientImplTest.java | 45 ++++++++++--------- .../client/GcsFileSystemImplTest.java | 7 ++- .../client/GcsFileSystemOptionsTest.java | 2 +- .../client/FakeGcsFileSystemImpl.java | 10 +---- 7 files changed, 32 insertions(+), 45 deletions(-) diff --git a/CONFIGURATION.md b/CONFIGURATION.md index ad61357fe..60123315a 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -49,14 +49,6 @@ These parameters fine-tune the low-level data streaming behavior. They allow you | `analytics-core.adaptive-read.sequential-read-threshold` | Threshold for number of sequential reads to switch to sequential mode. | `3` | | `analytics-core.random-read.min-request-size` | Minimum request size for random reads. If the requested read size is smaller, it reads up to this size. | `131072` (128 KB) | -### Metadata and Directory Operations - -These properties configure how the library handles file system metadata operations like directory creation, listing or file status retrieval. - -| Property | Description | Default Value | -| :--- | :--- | :--- | -| `analytics-core.hns.api.enable` | Controls whether the Hierarchical Namespace (HNS) API is enabled for operations. | `false` | - ### Telemetry and Monitoring These settings enable the emission of deep internal metrics—such as cache hit rates, operational durations, and throughput—to local logging consoles or distributed OpenTelemetry backends like Google Cloud Monitoring. diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java index 76d09a5de..e0061823d 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java @@ -25,7 +25,7 @@ public abstract class GcsFileSystemOptions { private static final String READ_THREAD_COUNT_KEY = "analytics-core.read.thread.count"; private static final String CLIENT_TYPE_KEY = "client.type"; - private static final String HNS_API_ENABLED_KEY = "analytics-core.hns.api.enable"; + private static final String HNS_API_ENABLED_KEY = "analytics-core.hierarchical.namespace.enable"; /** Cloud Storage client to use. */ public enum ClientType { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java index 0512a1bf8..9f896b7f4 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/NamespaceStrategy.java @@ -17,7 +17,8 @@ package com.google.cloud.gcs.analyticscore.client; /** - * Strategy interface for namespace operations. + * Strategy interface for handling directory operations across different namespace models (Flat vs. + * HNS). * *

Methods for directory operations will be added in follow-up PRs. These methods will include: * diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java index 35c5ca6a2..4a105af60 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java @@ -83,7 +83,10 @@ class GcsClientImplTest { private static final String TEST_OBJECT_ID = "test-object-id"; private static final String TEST_WRITE_OBJECT = "test-write-object"; private static final String TEST_NULL_OPTIONS_OBJECT = "test-null-options"; - private static final String TEST_NON_EXISTENT_OBJECT = "non-existent"; + private static final String TEST_NON_EXISTENT_OBJECT = "non-existent-object"; + private static final String TEST_HNS_BUCKET = "hns-bucket"; + private static final String TEST_FLAT_BUCKET = "flat-bucket"; + private static final String TEST_NON_EXISTENT_BUCKET = "non-existent-bucket"; private static final String TEST_OBJECT_NAME = "test-object-name"; private static final String BLOB_WRITE_SESSION_CONFIG_FIELD = "blobWriteSessionConfig"; private static final int MB = 1024 * 1024; @@ -312,63 +315,61 @@ void getBucketProperties_nullBucketName_throwsNullPointerException() { } @Test - void getBucketProperties_hnsEnabled_returnsTrue() throws IOException { + void getBucketProperties_hnsBucket_returnsTrue() throws IOException { Storage mockStorage = mock(Storage.class); GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); Bucket mockBucket = mockBucketWithHns(true); - doReturn(mockBucket).when(mockStorage).get(eq("hns-bucket"), any(BucketGetOption.class)); + doReturn(mockBucket).when(mockStorage).get(eq(TEST_HNS_BUCKET), any(BucketGetOption.class)); - BucketProperties bucketProperties = localGcsClient.getBucketProperties("hns-bucket"); + BucketProperties bucketProperties = localGcsClient.getBucketProperties(TEST_HNS_BUCKET); assertThat(bucketProperties.isHnsEnabled()).isTrue(); } @Test - void isHnsBucket_hnsEnabled_returnsTrue() throws IOException { + void isHnsBucket_hnsBucket_returnsTrue() throws IOException { Storage mockStorage = mock(Storage.class); GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); Bucket mockBucket = mockBucketWithHns(true); - doReturn(mockBucket).when(mockStorage).get(eq("hns-bucket"), any(BucketGetOption.class)); + doReturn(mockBucket).when(mockStorage).get(eq(TEST_HNS_BUCKET), any(BucketGetOption.class)); - boolean isHns = localGcsClient.isHnsBucket("hns-bucket"); + boolean isHns = localGcsClient.isHnsBucket(TEST_HNS_BUCKET); assertThat(isHns).isTrue(); } @Test - void getBucketProperties_hnsDisabled_returnsFalse() throws IOException { + void getBucketProperties_flatBucket_returnsFalse() throws IOException { Storage mockStorage = mock(Storage.class); GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); Bucket mockBucket = mockBucketWithHns(false); - doReturn(mockBucket).when(mockStorage).get(eq("flat-bucket"), any(BucketGetOption.class)); + doReturn(mockBucket).when(mockStorage).get(eq(TEST_FLAT_BUCKET), any(BucketGetOption.class)); - BucketProperties bucketProperties = localGcsClient.getBucketProperties("flat-bucket"); + BucketProperties bucketProperties = localGcsClient.getBucketProperties(TEST_FLAT_BUCKET); assertThat(bucketProperties.isHnsEnabled()).isFalse(); } @Test - void isHnsBucket_hnsDisabled_returnsFalse() throws IOException { + void isHnsBucket_flatBucket_returnsFalse() throws IOException { Storage mockStorage = mock(Storage.class); GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); Bucket mockBucket = mockBucketWithHns(false); - doReturn(mockBucket).when(mockStorage).get(eq("flat-bucket"), any(BucketGetOption.class)); + doReturn(mockBucket).when(mockStorage).get(eq(TEST_FLAT_BUCKET), any(BucketGetOption.class)); - boolean isHns = localGcsClient.isHnsBucket("flat-bucket"); + boolean isHns = localGcsClient.isHnsBucket(TEST_FLAT_BUCKET); assertThat(isHns).isFalse(); } @Test - void getBucketProperties_hnsNull_returnsFalse() throws IOException { + void getBucketProperties_missingHnsProperty_returnsFalse() throws IOException { Storage mockStorage = mock(Storage.class); GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); Bucket mockBucket = mockBucketWithHns(null); - doReturn(mockBucket) - .when(mockStorage) - .get(eq("flat-bucket-null-hns"), any(BucketGetOption.class)); + doReturn(mockBucket).when(mockStorage).get(eq(TEST_BUCKET), any(BucketGetOption.class)); - BucketProperties bucketProperties = localGcsClient.getBucketProperties("flat-bucket-null-hns"); + BucketProperties bucketProperties = localGcsClient.getBucketProperties(TEST_BUCKET); assertThat(bucketProperties.isHnsEnabled()).isFalse(); } @@ -377,9 +378,9 @@ void getBucketProperties_hnsNull_returnsFalse() throws IOException { void getBucketProperties_bucketNotFound_returnsDisabledHns() throws Exception { Storage mockStorage = mock(Storage.class); GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); - doReturn(null).when(mockStorage).get(eq("non-existent-bucket"), any(BucketGetOption.class)); + doReturn(null).when(mockStorage).get(eq(TEST_NON_EXISTENT_BUCKET), any(BucketGetOption.class)); - BucketProperties properties = localGcsClient.getBucketProperties("non-existent-bucket"); + BucketProperties properties = localGcsClient.getBucketProperties(TEST_NON_EXISTENT_BUCKET); assertThat(properties.isHnsEnabled()).isFalse(); } @@ -532,11 +533,11 @@ void create_whenBucketOrObjectNotFound_throwsFileNotFoundException() throws Exce GcsClientImpl clientWithMock = createClientWithMockStorage(mockStorage); GcsItemId itemId = GcsItemId.builder() - .setBucketName("non-existent-bucket") + .setBucketName(TEST_NON_EXISTENT_BUCKET) .setObjectName("test-object") .build(); BlobInfo blobInfo = - BlobInfo.newBuilder(BlobId.of("non-existent-bucket", "test-object")) + BlobInfo.newBuilder(BlobId.of(TEST_NON_EXISTENT_BUCKET, "test-object")) .setContentType("application/octet-stream") .build(); StorageException e404 = new StorageException(404, "Not Found"); diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 1480bda00..888381718 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -594,7 +594,7 @@ void create_nullWriteOptions_delegatesToClientWithNullOptions() throws IOExcepti } @Test - void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() throws IOException { + void resolveStrategy_hnsFlagEnabledAndHnsBucket_returnsHnsStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) @@ -610,7 +610,7 @@ void resolveStrategy_hnsEnabledInPropertiesAndApiEnabled_returnsHnsStrategy() th } @Test - void resolveStrategy_hnsApiDisabled_returnsFlatStrategy() throws IOException { + void resolveStrategy_hnsFlagDisabled_returnsFlatStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) @@ -625,8 +625,7 @@ void resolveStrategy_hnsApiDisabled_returnsFlatStrategy() throws IOException { } @Test - void resolveStrategy_hnsDisabledInPropertiesAndApiEnabled_returnsFlatStrategy() - throws IOException { + void resolveStrategy_hnsFlagEnabledAndFlatBucket_returnsFlatStrategy() throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java index 1fbf99852..a21fa52dd 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java @@ -33,7 +33,7 @@ void createFromOptions_withValidProperties_shouldCreateCorrectOptions() { "fs.gs.project-id", "test-project", "fs.gs.client.type", "GRPC_CLIENT", "fs.gs.analytics-core.read.thread.count", "32", - "fs.gs.analytics-core.hns.api.enable", "true"); + "fs.gs.analytics-core.hierarchical.namespace.enable", "true"); GcsFileSystemOptions options = GcsFileSystemOptions.createFromOptions(properties, "fs.gs."); diff --git a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java index bd66091ae..a1f081380 100644 --- a/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java +++ b/test-lib/src/main/java/com/google/cloud/gcs/analyticscore/client/FakeGcsFileSystemImpl.java @@ -29,20 +29,14 @@ public FakeGcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { } private FakeGcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions, Telemetry telemetry) { - this(fileSystemOptions, telemetry, initializeGcsClient(fileSystemOptions, telemetry)); - } - - private FakeGcsFileSystemImpl( - GcsFileSystemOptions fileSystemOptions, Telemetry telemetry, FakeGcsClientImpl fakeClient) { super( - fakeClient, + initializeGcsClient(fileSystemOptions, telemetry), fileSystemOptions, telemetry, new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions())); } - private static FakeGcsClientImpl initializeGcsClient( - GcsFileSystemOptions options, Telemetry telemetry) { + private static GcsClient initializeGcsClient(GcsFileSystemOptions options, Telemetry telemetry) { Supplier executorServiceSupplier = Suppliers.ofInstance(Executors.newCachedThreadPool()); return new FakeGcsClientImpl(options.getGcsClientOptions(), executorServiceSupplier, telemetry); From b2a3c6fcae13072cc050e8b074ee181765dbc723 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 5 Aug 2026 11:26:08 +0000 Subject: [PATCH 17/28] test: refactor GcsFileSystemImplTest to improve test readability and follow AAA structure --- .../analyticscore/client/GcsFileSystemImplTest.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 888381718..71b6510f8 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -141,10 +141,8 @@ void constructor_withValidOptions_passesMemorizedExecutorServiceAndTelemetryToGc assertThat(capturedSupplier.get()).isNotNull(); assertThat(capturedSupplier.get().get()).isNotNull(); assertThat(executorService1).isEqualTo(executorService2); - assertThat(capturedTelemetry.get()).isNotNull(); assertThat(capturedTelemetry.get()).isSameInstanceAs(fs.getTelemetry()); - assertThat(fs.getFlatStrategy()).isNotNull(); assertThat(fs.getHnsStrategy()).isNotNull(); } @@ -600,9 +598,9 @@ void resolveStrategy_hnsFlagEnabledAndHnsBucket_returnsHnsStrategy() throws IOEx .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(true) .build(); - try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.isHnsBucket(TEST_BUCKET)).thenReturn(true); + when(mockClient.isHnsBucket(TEST_BUCKET)).thenReturn(true); + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { NamespaceStrategy strategy = gcsFileSystem.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(HierarchicalNamespaceStrategyImpl.class); @@ -616,6 +614,7 @@ void resolveStrategy_hnsFlagDisabled_returnsFlatStrategy() throws IOException { .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(false) .build(); + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { NamespaceStrategy strategy = gcsFileSystem.resolveStrategy(TEST_BUCKET); @@ -631,9 +630,9 @@ void resolveStrategy_hnsFlagEnabledAndFlatBucket_returnsFlatStrategy() throws IO .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) .setHnsApiEnabled(true) .build(); - try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { - when(mockClient.isHnsBucket(TEST_BUCKET)).thenReturn(false); + when(mockClient.isHnsBucket(TEST_BUCKET)).thenReturn(false); + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { NamespaceStrategy strategy = gcsFileSystem.resolveStrategy(TEST_BUCKET); assertThat(strategy).isInstanceOf(FlatNamespaceStrategyImpl.class); From 5e53b883112fcba78f56aedebca0408a0c1b39e9 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 5 Aug 2026 12:25:58 +0000 Subject: [PATCH 18/28] fix: wrap IOException in UncheckedIOException during bucket property resolution in GcsFileSystemImpl --- .../client/GcsFileSystemImpl.java | 10 +++++++++- .../client/GcsFileSystemImplTest.java | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index e3d54eb34..dc127eb2f 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ThreadFactoryBuilder; import java.io.IOException; +import java.io.UncheckedIOException; import java.net.URI; import java.nio.channels.WritableByteChannel; import java.util.Collections; @@ -124,7 +125,14 @@ NamespaceStrategy resolveStrategy(String bucketName) throws IOException { BucketProperties properties = cacheManager.getBucketProperties( - bucketName, name -> BucketProperties.create(gcsClient.isHnsBucket(name))); + bucketName, + name -> { + try { + return BucketProperties.create(gcsClient.isHnsBucket(name)); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); if (properties.isHnsEnabled()) { return hnsStrategy; diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 71b6510f8..8124f1ff4 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -35,6 +35,7 @@ import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import java.io.IOException; +import java.io.UncheckedIOException; import java.net.URI; import java.net.URISyntaxException; import java.nio.channels.WritableByteChannel; @@ -639,6 +640,25 @@ void resolveStrategy_hnsFlagEnabledAndFlatBucket_returnsFlatStrategy() throws IO } } + @Test + void resolveStrategy_isHnsBucketThrowsIoException_throwsUncheckedIOException() throws IOException { + GcsFileSystemOptions options = + GcsFileSystemOptions.builder() + .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) + .setHnsApiEnabled(true) + .build(); + when(mockClient.isHnsBucket(TEST_BUCKET)).thenThrow(new IOException("test exception")); + + try (GcsFileSystemImpl gcsFileSystem = new GcsFileSystemImpl(mockClient, options)) { + UncheckedIOException exception = + assertThrows( + UncheckedIOException.class, () -> gcsFileSystem.resolveStrategy(TEST_BUCKET)); + + assertThat(exception).hasCauseThat().isInstanceOf(IOException.class); + assertThat(exception).hasCauseThat().hasMessageThat().isEqualTo("test exception"); + } + } + @SuppressWarnings("unchecked") private List getRegisteredTelemetryListeners(Telemetry telemetry) { try { From fa451d66bc6b230f5c76f4f309d7c7ac788b51f0 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 5 Aug 2026 14:49:36 +0000 Subject: [PATCH 19/28] feat: enable HNS API by default in GcsFileSystemOptions --- .../cloud/gcs/analyticscore/client/GcsFileSystemOptions.java | 2 +- .../cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java | 3 ++- .../gcs/analyticscore/client/GcsFileSystemOptionsTest.java | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java index e0061823d..3ba3a0435 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java @@ -52,7 +52,7 @@ public static Builder builder() { return new AutoValue_GcsFileSystemOptions.Builder() .setReadThreadCount(16) .setClientType(ClientType.HTTP_CLIENT) - .setHnsApiEnabled(false) + .setHnsApiEnabled(true) .setGcsClientOptions(GcsClientOptions.builder().build()) .setGcsCacheOptions(GcsCacheOptions.builder().build()) .setAnalyticsCoreTelemetryOptions(TelemetryOptions.builder().build()); diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 8124f1ff4..e333d77ab 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -641,7 +641,8 @@ void resolveStrategy_hnsFlagEnabledAndFlatBucket_returnsFlatStrategy() throws IO } @Test - void resolveStrategy_isHnsBucketThrowsIoException_throwsUncheckedIOException() throws IOException { + void resolveStrategy_isHnsBucketThrowsIoException_throwsUncheckedIOException() + throws IOException { GcsFileSystemOptions options = GcsFileSystemOptions.builder() .setGcsClientOptions(TEST_GCS_CLIENT_OPTIONS) diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java index a21fa52dd..c919b3fa0 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java @@ -74,7 +74,7 @@ void createFromOptions_withDefaultProperties_shouldCreateCorrectOptions() { assertThat(options.getGcsClientOptions().getProjectId().isEmpty()).isTrue(); assertThat(options.getClientType()).isEqualTo(GcsFileSystemOptions.ClientType.HTTP_CLIENT); assertThat(options.getReadThreadCount()).isEqualTo(16); - assertThat(options.isHnsApiEnabled()).isFalse(); + assertThat(options.isHnsApiEnabled()).isTrue(); GcsCacheOptions cacheOptions = options.getGcsCacheOptions(); assertThat(cacheOptions.isFooterCacheEnabled()).isFalse(); From 368252e14ee06f921aeebf5849711b42c3c93d38 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 7 Jul 2026 09:43:00 +0000 Subject: [PATCH 20/28] feat: introduce LazyExecutorService and configurable parallel list operation --- .../client/FlatNamespaceStrategyImpl.java | 9 +- .../client/GcsFileSystemImpl.java | 73 +++++-- .../client/GcsFileSystemOptions.java | 10 + .../client/LazyExecutorService.java | 124 ++++++++++++ .../client/GcsFileSystemImplTest.java | 120 +++++++++--- .../client/LazyExecutorServiceTest.java | 181 ++++++++++++++++++ 6 files changed, 470 insertions(+), 47 deletions(-) create mode 100644 client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java create mode 100644 client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java index fe5e203cc..6e9102b20 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java @@ -16,10 +16,17 @@ package com.google.cloud.gcs.analyticscore.client; +import com.google.common.base.Supplier; +import java.util.concurrent.ExecutorService; + final class FlatNamespaceStrategyImpl implements NamespaceStrategy { + private final GcsClient gcsClient; + private final Supplier listExecutorServiceSupplier; - FlatNamespaceStrategyImpl(GcsClient gcsClient) { + FlatNamespaceStrategyImpl( + GcsClient gcsClient, Supplier listExecutorServiceSupplier) { this.gcsClient = gcsClient; + this.listExecutorServiceSupplier = listExecutorServiceSupplier; } } diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index dc127eb2f..92184e7e3 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -47,7 +47,8 @@ public class GcsFileSystemImpl implements GcsFileSystem { private final GcsClient gcsClient; private final GcsFileSystemOptions fileSystemOptions; - private final Supplier executorServiceSupplier; + private final Supplier readExecutorServiceSupplier; + private final Supplier listExecutorServiceSupplier; private final Telemetry telemetry; private final AnalyticsCacheManager cacheManager; @@ -57,7 +58,8 @@ public class GcsFileSystemImpl implements GcsFileSystem { public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { this.fileSystemOptions = fileSystemOptions; - this.executorServiceSupplier = initializeExecutionServiceSupplier(); + this.readExecutorServiceSupplier = initializeReadExecutionServiceSupplier(); + this.listExecutorServiceSupplier = initializeListExecutionServiceSupplier(); this.telemetry = createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()); this.cacheManager = new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()); this.gcsClient = @@ -67,14 +69,18 @@ public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { Collections.emptyMap(), recorder -> new GcsClientImpl( - fileSystemOptions.getGcsClientOptions(), executorServiceSupplier, telemetry)); - this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); + fileSystemOptions.getGcsClientOptions(), + readExecutorServiceSupplier, + telemetry)); + this.flatStrategy = + new FlatNamespaceStrategyImpl(this.gcsClient, this.listExecutorServiceSupplier); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSystemOptions) { this.fileSystemOptions = fileSystemOptions; - this.executorServiceSupplier = initializeExecutionServiceSupplier(); + this.readExecutorServiceSupplier = initializeReadExecutionServiceSupplier(); + this.listExecutorServiceSupplier = initializeListExecutionServiceSupplier(); this.telemetry = createTelemetry(fileSystemOptions.getAnalyticsCoreTelemetryOptions()); this.cacheManager = new AnalyticsCacheManager(fileSystemOptions.getGcsCacheOptions()); this.gcsClient = @@ -86,9 +92,10 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste new GcsClientImpl( credentials, fileSystemOptions.getGcsClientOptions(), - executorServiceSupplier, + readExecutorServiceSupplier, telemetry)); - this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); + this.flatStrategy = + new FlatNamespaceStrategyImpl(this.gcsClient, this.listExecutorServiceSupplier); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -109,10 +116,12 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste AnalyticsCacheManager cacheManager) { this.gcsClient = gcsClient; this.fileSystemOptions = fileSystemOptions; - this.executorServiceSupplier = initializeExecutionServiceSupplier(); + this.readExecutorServiceSupplier = initializeReadExecutionServiceSupplier(); + this.listExecutorServiceSupplier = initializeListExecutionServiceSupplier(); this.telemetry = telemetry; this.cacheManager = cacheManager; - this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); + this.flatStrategy = + new FlatNamespaceStrategyImpl(this.gcsClient, this.listExecutorServiceSupplier); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -209,14 +218,19 @@ HierarchicalNamespaceStrategyImpl getHnsStrategy() { @Override public void close() { - ExecutorService executorService = executorServiceSupplier.get(); - executorService.shutdown(); + ExecutorService readExecutorService = readExecutorServiceSupplier.get(); + ExecutorService listExecutorService = listExecutorServiceSupplier.get(); + readExecutorService.shutdown(); + listExecutorService.shutdown(); try { - if (!executorService.awaitTermination(10, TimeUnit.SECONDS)) { - executorService.shutdownNow(); + if (!readExecutorService.awaitTermination(10, TimeUnit.SECONDS) + || !listExecutorService.awaitTermination(10, TimeUnit.SECONDS)) { + readExecutorService.shutdownNow(); + listExecutorService.shutdownNow(); } } catch (InterruptedException e) { - executorService.shutdownNow(); + readExecutorService.shutdownNow(); + listExecutorService.shutdownNow(); Thread.currentThread().interrupt(); } gcsClient.close(); @@ -250,7 +264,7 @@ static Telemetry createTelemetry(TelemetryOptions telemetryOptions) { } @VisibleForTesting - Supplier initializeExecutionServiceSupplier() { + Supplier initializeReadExecutionServiceSupplier() { return Suppliers.memoize( () -> new ThreadPoolExecutor( @@ -264,4 +278,33 @@ Supplier initializeExecutionServiceSupplier() { .setDaemon(true) .build())); } + + @VisibleForTesting + Supplier initializeListExecutionServiceSupplier() { + return Suppliers.memoize( + () -> { + if (fileSystemOptions.isListParallelEnabled()) { + return createCachedExecutor(); + } + return new LazyExecutorService(); + }); + } + + private static ExecutorService createCachedExecutor() { + ThreadPoolExecutor service = + new ThreadPoolExecutor( + /* corePoolSize= */ 2, + /* maximumPoolSize= */ Integer.MAX_VALUE, + /* keepAliveTime= */ 30, + TimeUnit.SECONDS, + new java.util.concurrent.SynchronousQueue<>(), + new ThreadFactoryBuilder() + .setNameFormat("gcs-filesystem-list-pool-%d") + .setDaemon(true) + .build()); + // allowCoreThreadTimeOut needs to be enabled for cases where the encapsulating class does not + // properly shut down the executor, preventing thread leaks. + service.allowCoreThreadTimeOut(true); + return service; + } } diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java index 3ba3a0435..31456e7b3 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptions.java @@ -26,6 +26,7 @@ public abstract class GcsFileSystemOptions { private static final String READ_THREAD_COUNT_KEY = "analytics-core.read.thread.count"; private static final String CLIENT_TYPE_KEY = "client.type"; private static final String HNS_API_ENABLED_KEY = "analytics-core.hierarchical.namespace.enable"; + private static final String LIST_PARALLEL_ENABLED_KEY = "analytics-core.list.parallel.enabled"; /** Cloud Storage client to use. */ public enum ClientType { @@ -46,6 +47,8 @@ public enum ClientType { public abstract boolean isHnsApiEnabled(); + public abstract boolean isListParallelEnabled(); + public abstract Builder toBuilder(); public static Builder builder() { @@ -53,6 +56,7 @@ public static Builder builder() { .setReadThreadCount(16) .setClientType(ClientType.HTTP_CLIENT) .setHnsApiEnabled(true) + .setListParallelEnabled(true) .setGcsClientOptions(GcsClientOptions.builder().build()) .setGcsCacheOptions(GcsCacheOptions.builder().build()) .setAnalyticsCoreTelemetryOptions(TelemetryOptions.builder().build()); @@ -73,6 +77,10 @@ public static GcsFileSystemOptions createFromOptions( optionsBuilder.setHnsApiEnabled( Boolean.parseBoolean(analyticsCoreOptions.get(prefix + HNS_API_ENABLED_KEY))); } + if (analyticsCoreOptions.containsKey(prefix + LIST_PARALLEL_ENABLED_KEY)) { + optionsBuilder.setListParallelEnabled( + Boolean.parseBoolean(analyticsCoreOptions.get(prefix + LIST_PARALLEL_ENABLED_KEY))); + } optionsBuilder.setGcsClientOptions( GcsClientOptions.createFromOptions(analyticsCoreOptions, prefix)); @@ -95,6 +103,8 @@ public abstract static class Builder { public abstract Builder setHnsApiEnabled(boolean isHnsApiEnabled); + public abstract Builder setListParallelEnabled(boolean isListParallelEnabled); + public abstract Builder setGcsClientOptions(GcsClientOptions gcsClientOptions); /** Sets the configuration options for the GCS caching layer. */ diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java new file mode 100644 index 000000000..a64be2076 --- /dev/null +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -0,0 +1,124 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client; + +import java.util.Collections; +import java.util.List; +import java.util.concurrent.AbstractExecutorService; +import java.util.concurrent.Callable; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.FutureTask; +import java.util.concurrent.RunnableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * A lightweight, lazy ExecutorService that defers task execution until Future.get() is called. + * Execution happens synchronously on the thread that calls get(). + */ +public class LazyExecutorService extends AbstractExecutorService { + + private volatile boolean isShutdown = false; + + @Override + public void shutdown() { + isShutdown = true; + } + + @Override + public List shutdownNow() { + isShutdown = true; + return Collections.emptyList(); + } + + @Override + public boolean isShutdown() { + return isShutdown; + } + + @Override + public boolean isTerminated() { + return isShutdown; + } + + @Override + public boolean awaitTermination(long timeout, TimeUnit unit) { + return true; + } + + @Override + public void execute(Runnable command) { + // Intentional no-op. Execution is deferred until get() is called on the Future. + } + + @Override + protected RunnableFuture newTaskFor(Callable callable) { + return new FutureTask(callable) { + @Override + public T get() throws InterruptedException, ExecutionException { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } + if (!isDone() && !isCancelled()) { + run(); // Execute on the caller's thread when get() is called + } + return super.get(); + } + + @Override + public T get(long timeout, TimeUnit unit) + throws InterruptedException, ExecutionException, TimeoutException { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } + if (!isDone() && !isCancelled()) { + run(); + } + return super.get(timeout, unit); + } + }; + } + + @Override + protected RunnableFuture newTaskFor(Runnable runnable, T value) { + return new FutureTask(runnable, value) { + @Override + public T get() throws InterruptedException, ExecutionException { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } + if (!isDone() && !isCancelled()) { + run(); + } + return super.get(); + } + + @Override + public T get(long timeout, TimeUnit unit) + throws InterruptedException, ExecutionException, TimeoutException { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } + if (!isDone() && !isCancelled()) { + run(); + } + return super.get(timeout, unit); + } + }; + } +} diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index e333d77ab..0313ed152 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -316,80 +316,138 @@ void getFileInfo_withNonExistentItemId_shouldThrowException() throws IOException } @Test - void initializeExecutionServiceSupplier_shouldReturnMemoizedExecutorService() { + void initializeReadExecutionServiceSupplier_shouldReturnMemoizedExecutorService() { GcsFileSystemImpl fileSystemImpl = (GcsFileSystemImpl) gcsFileSystem; - Supplier executorServiceSupplier = - fileSystemImpl.initializeExecutionServiceSupplier(); + Supplier readExecutorServiceSupplier = + fileSystemImpl.initializeReadExecutionServiceSupplier(); - assertThat(executorServiceSupplier).isNotNull(); - assertThat(executorServiceSupplier.get()).isNotNull(); - assertThat(executorServiceSupplier.get()).isInstanceOf(ThreadPoolExecutor.class); - assertThat(((ThreadPoolExecutor) executorServiceSupplier.get()).getCorePoolSize()) + assertThat(readExecutorServiceSupplier).isNotNull(); + assertThat(readExecutorServiceSupplier.get()).isNotNull(); + assertThat(readExecutorServiceSupplier.get()).isInstanceOf(ThreadPoolExecutor.class); + assertThat(((ThreadPoolExecutor) readExecutorServiceSupplier.get()).getCorePoolSize()) .isEqualTo(16); } + @Test + void + initializeListExecutionServiceSupplier_whenListParallelEnabled_shouldReturnCachedExecutorService() { + GcsFileSystemImpl fileSystemImpl = (GcsFileSystemImpl) gcsFileSystem; + + Supplier listExecutorServiceSupplier = + fileSystemImpl.initializeListExecutionServiceSupplier(); + + assertThat(listExecutorServiceSupplier).isNotNull(); + assertThat(listExecutorServiceSupplier.get()).isNotNull(); + assertThat(listExecutorServiceSupplier.get()).isInstanceOf(ThreadPoolExecutor.class); + } + + @Test + void + initializeListExecutionServiceSupplier_whenListParallelDisabled_shouldReturnLazyExecutorService() + throws Exception { + GcsFileSystemOptions options = + TEST_GCS_FILESYSTEM_OPTIONS.toBuilder().setListParallelEnabled(false).build(); + // Use try-with-resources to ensure GcsFileSystemImpl is closed with its internal executors. + try (GcsFileSystemImpl fileSystemImpl = new GcsFileSystemImpl(mock(GcsClient.class), options)) { + + Supplier listExecutorServiceSupplier = + fileSystemImpl.initializeListExecutionServiceSupplier(); + + assertThat(listExecutorServiceSupplier).isNotNull(); + assertThat(listExecutorServiceSupplier.get()).isNotNull(); + assertThat(listExecutorServiceSupplier.get()).isInstanceOf(LazyExecutorService.class); + } + } + @Test void close_whenTerminationSucceeds_shutsDownGracefully() throws InterruptedException { - ExecutorService mockExecutorService = mock(ExecutorService.class); - when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(true); + ExecutorService mockReadExecutorService = mock(ExecutorService.class); + ExecutorService mockListExecutorService = mock(ExecutorService.class); + when(mockReadExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(true); + when(mockListExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(true); GcsFileSystemImpl fileSystemWithMockExecutor = new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override - Supplier initializeExecutionServiceSupplier() { - return () -> mockExecutorService; + Supplier initializeReadExecutionServiceSupplier() { + return () -> mockReadExecutorService; + } + + @Override + Supplier initializeListExecutionServiceSupplier() { + return () -> mockListExecutorService; } }; fileSystemWithMockExecutor.close(); - InOrder inOrder = inOrder(mockExecutorService, mockClient); + InOrder inOrder = inOrder(mockReadExecutorService, mockListExecutorService, mockClient); - inOrder.verify(mockExecutorService).shutdown(); - inOrder.verify(mockExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); + inOrder.verify(mockReadExecutorService).shutdown(); + inOrder.verify(mockListExecutorService).shutdown(); + inOrder.verify(mockReadExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); + inOrder.verify(mockListExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); inOrder.verify(mockClient).close(); - verify(mockExecutorService, never()).shutdownNow(); + verify(mockReadExecutorService, never()).shutdownNow(); + verify(mockListExecutorService, never()).shutdownNow(); } @Test void close_whenTerminationTimesOut_shutsDownNow() throws InterruptedException { - ExecutorService mockExecutorService = mock(ExecutorService.class); - when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))).thenReturn(false); + ExecutorService mockReadExecutorService = mock(ExecutorService.class); + ExecutorService mockListExecutorService = mock(ExecutorService.class); + when(mockReadExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) + .thenReturn(false); GcsFileSystemImpl fileSystemWithMockExecutor = new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override - Supplier initializeExecutionServiceSupplier() { - return () -> mockExecutorService; + Supplier initializeReadExecutionServiceSupplier() { + return () -> mockReadExecutorService; + } + + @Override + Supplier initializeListExecutionServiceSupplier() { + return () -> mockListExecutorService; } }; fileSystemWithMockExecutor.close(); - InOrder inOrder = inOrder(mockExecutorService, mockClient); + InOrder inOrder = inOrder(mockReadExecutorService, mockListExecutorService, mockClient); - inOrder.verify(mockExecutorService).shutdown(); - inOrder.verify(mockExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); - inOrder.verify(mockExecutorService).shutdownNow(); + inOrder.verify(mockReadExecutorService).shutdown(); + inOrder.verify(mockListExecutorService).shutdown(); + inOrder.verify(mockReadExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); + inOrder.verify(mockReadExecutorService).shutdownNow(); + inOrder.verify(mockListExecutorService).shutdownNow(); inOrder.verify(mockClient).close(); } @Test void close_whenInterrupted_reInterruptsThreadAndShutsDownNow() throws InterruptedException { - ExecutorService mockExecutorService = mock(ExecutorService.class); - when(mockExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) + ExecutorService mockReadExecutorService = mock(ExecutorService.class); + ExecutorService mockListExecutorService = mock(ExecutorService.class); + when(mockReadExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) .thenThrow(new InterruptedException()); GcsFileSystemImpl fileSystemWithMockExecutor = new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override - Supplier initializeExecutionServiceSupplier() { - return () -> mockExecutorService; + Supplier initializeReadExecutionServiceSupplier() { + return () -> mockReadExecutorService; + } + + @Override + Supplier initializeListExecutionServiceSupplier() { + return () -> mockListExecutorService; } }; fileSystemWithMockExecutor.close(); - InOrder inOrder = inOrder(mockExecutorService, mockClient); + InOrder inOrder = inOrder(mockReadExecutorService, mockListExecutorService, mockClient); - inOrder.verify(mockExecutorService).shutdown(); - inOrder.verify(mockExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); - inOrder.verify(mockExecutorService).shutdownNow(); + inOrder.verify(mockReadExecutorService).shutdown(); + inOrder.verify(mockListExecutorService).shutdown(); + inOrder.verify(mockReadExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); + inOrder.verify(mockReadExecutorService).shutdownNow(); + inOrder.verify(mockListExecutorService).shutdownNow(); inOrder.verify(mockClient).close(); assertThat(Thread.currentThread().isInterrupted()).isTrue(); Thread.interrupted(); // Clear interrupted status to not affect other tests diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java new file mode 100644 index 000000000..270080d87 --- /dev/null +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -0,0 +1,181 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.gcs.analyticscore.client; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import java.util.concurrent.Callable; +import java.util.concurrent.CancellationException; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class LazyExecutorServiceTest { + + private LazyExecutorService executorService; + + @Before + public void setUp() { + executorService = new LazyExecutorService(); + } + + @Test + public void testSubmitCallableIsLazyAndRunsOnCallerThread() throws Exception { + AtomicBoolean executed = new AtomicBoolean(false); + AtomicReference executionThread = new AtomicReference<>(); + + Callable task = + () -> { + executed.set(true); + executionThread.set(Thread.currentThread()); + return "success"; + }; + + Future future = executorService.submit(task); + + // Verify it has not executed yet + assertThat(executed.get()).isFalse(); + + // Call get() to trigger execution + String result = future.get(); + + // Verify execution happened and result is correct + assertThat(executed.get()).isTrue(); + assertThat(result).isEqualTo("success"); + + // Verify it ran on the caller's thread + assertThat(executionThread.get()).isEqualTo(Thread.currentThread()); + } + + @Test + public void testSubmitRunnableIsLazy() throws Exception { + AtomicBoolean executed = new AtomicBoolean(false); + + Runnable task = () -> executed.set(true); + + Future future = executorService.submit(task); + + // Verify it has not executed yet + assertThat(executed.get()).isFalse(); + + // Call get() to trigger execution + future.get(); + + // Verify execution happened + assertThat(executed.get()).isTrue(); + } + + @Test + public void testShutdownThrowsCancellationExceptionOnGet() { + AtomicBoolean executed = new AtomicBoolean(false); + Callable task = + () -> { + executed.set(true); + return "success"; + }; + + Future future = executorService.submit(task); + assertThat(executed.get()).isFalse(); + + // Shutdown the executor + executorService.shutdown(); + assertThat(executorService.isShutdown()).isTrue(); + + // Call get() and expect CancellationException + assertThrows(CancellationException.class, future::get); + + // Verify task was never executed + assertThat(executed.get()).isFalse(); + } + + @Test + public void testShutdownNowReturnsEmptyListAndCancelsFutureTaskExecution() { + AtomicBoolean executed = new AtomicBoolean(false); + Callable task = + () -> { + executed.set(true); + return "success"; + }; + + Future future = executorService.submit(task); + + // shutdownNow should return an empty list since we don't track tasks + assertThat(executorService.shutdownNow()).isEmpty(); + assertThat(executorService.isShutdown()).isTrue(); + + assertThrows(CancellationException.class, future::get); + assertThat(executed.get()).isFalse(); + } + + @Test + public void testSubmitCallableIsLazyWithTimeout() throws Exception { + AtomicBoolean executed = new AtomicBoolean(false); + Callable task = + () -> { + executed.set(true); + return "success"; + }; + Future future = executorService.submit(task); + assertThat(executed.get()).isFalse(); + + String result = future.get(10, java.util.concurrent.TimeUnit.SECONDS); + assertThat(executed.get()).isTrue(); + assertThat(result).isEqualTo("success"); + } + + @Test + public void testSubmitRunnableIsLazyWithTimeout() throws Exception { + AtomicBoolean executed = new AtomicBoolean(false); + Runnable task = () -> executed.set(true); + Future future = executorService.submit(task); + assertThat(executed.get()).isFalse(); + + future.get(10, java.util.concurrent.TimeUnit.SECONDS); + assertThat(executed.get()).isTrue(); + } + + @Test + public void testShutdownThrowsCancellationExceptionOnGetWithTimeout() { + AtomicBoolean executed = new AtomicBoolean(false); + Callable task = + () -> { + executed.set(true); + return "success"; + }; + Future future = executorService.submit(task); + executorService.shutdown(); + + assertThrows( + CancellationException.class, () -> future.get(10, java.util.concurrent.TimeUnit.SECONDS)); + assertThat(executed.get()).isFalse(); + } + + @Test + public void testAwaitTerminationAndIsTerminated() throws Exception { + assertThat(executorService.isTerminated()).isFalse(); + executorService.shutdown(); + assertThat(executorService.isTerminated()).isTrue(); + assertThat(executorService.awaitTermination(10, java.util.concurrent.TimeUnit.SECONDS)) + .isTrue(); + } +} From f870571691c9751ed0666ab77e00d068a0bc0a6a Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 7 Jul 2026 10:40:15 +0000 Subject: [PATCH 21/28] refactor: improve LazyExecutorService, add GcsFileSystem input validation and enhance test coverage --- .../client/GcsFileSystemImpl.java | 7 ++- .../client/LazyExecutorService.java | 48 ++++++++++++++----- .../client/GcsClientImplTest.java | 15 ++++++ .../client/GcsFileSystemOptionsTest.java | 10 ++++ .../client/LazyExecutorServiceTest.java | 24 ++++++++++ 5 files changed, 89 insertions(+), 15 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 92184e7e3..d0e82e58f 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -223,9 +223,12 @@ public void close() { readExecutorService.shutdown(); listExecutorService.shutdown(); try { - if (!readExecutorService.awaitTermination(10, TimeUnit.SECONDS) - || !listExecutorService.awaitTermination(10, TimeUnit.SECONDS)) { + boolean readTerminated = readExecutorService.awaitTermination(10, TimeUnit.SECONDS); + boolean listTerminated = listExecutorService.awaitTermination(10, TimeUnit.SECONDS); + if (!readTerminated) { readExecutorService.shutdownNow(); + } + if (!listTerminated) { listExecutorService.shutdownNow(); } } catch (InterruptedException e) { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index a64be2076..a45dd75a4 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -22,7 +22,10 @@ import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.FutureTask; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.RunnableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -63,7 +66,26 @@ public boolean awaitTermination(long timeout, TimeUnit unit) { @Override public void execute(Runnable command) { - // Intentional no-op. Execution is deferred until get() is called on the Future. + throw new RejectedExecutionException("Use submit instead of execute."); + } + + @Override + public Future submit(Runnable task) { + return submit(Executors.callable(task)); + } + + @Override + public Future submit(Runnable task, T result) { + return submit(Executors.callable(task, result)); + } + + @Override + public Future submit(Callable task) { + if (task == null) throw new NullPointerException(); + if (isShutdown) { + throw new RejectedExecutionException("Executor is shut down"); + } + return newTaskFor(task); } @Override @@ -71,10 +93,10 @@ protected RunnableFuture newTaskFor(Callable callable) { return new FutureTask(callable) { @Override public T get() throws InterruptedException, ExecutionException { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } if (!isDone() && !isCancelled()) { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } run(); // Execute on the caller's thread when get() is called } return super.get(); @@ -83,10 +105,10 @@ public T get() throws InterruptedException, ExecutionException { @Override public T get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } if (!isDone() && !isCancelled()) { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } run(); } return super.get(timeout, unit); @@ -99,10 +121,10 @@ protected RunnableFuture newTaskFor(Runnable runnable, T value) { return new FutureTask(runnable, value) { @Override public T get() throws InterruptedException, ExecutionException { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } if (!isDone() && !isCancelled()) { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } run(); } return super.get(); @@ -111,10 +133,10 @@ public T get() throws InterruptedException, ExecutionException { @Override public T get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } if (!isDone() && !isCancelled()) { + if (isShutdown) { + throw new CancellationException("Executor is shut down"); + } run(); } return super.get(timeout, unit); diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java index 0a229db7f..0d9f64aa4 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java @@ -1064,4 +1064,19 @@ protected Storage createStorage(Optional credentials) { assertThat(channel).isInstanceOf(GcsBidiReadChannel.class); } + + @Test + void getGcsItemInfo_storageThrowsStorageException_throwsIOException() { + Storage mockStorage = mock(Storage.class); + GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); + GcsItemId itemId = + GcsItemId.builder().setBucketName("test-bucket-name").setObjectName("test-object").build(); + doThrow(new StorageException(500, "Internal Error")) + .when(mockStorage) + .get(any(BlobId.class), any()); + + IOException e = assertThrows(IOException.class, () -> localGcsClient.getGcsItemInfo(itemId)); + + assertThat(e).hasMessageThat().contains("Unable to access blob"); + } } diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java index c919b3fa0..0f6d42157 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java @@ -82,4 +82,14 @@ void createFromOptions_withDefaultProperties_shouldCreateCorrectOptions() { assertThat(cacheOptions.isSmallObjectCacheEnabled()).isFalse(); assertThat(cacheOptions.getSmallObjectCacheMaxSizeBytes()).isEqualTo(200 * MB); } + + @Test + void createFromOptions_withStatusParallelEnabledFalse_createsCorrectOptions() { + ImmutableMap properties = + ImmutableMap.of("fs.gs.analytics-core.status.parallel.enabled", "false"); + + GcsFileSystemOptions options = GcsFileSystemOptions.createFromOptions(properties, "fs.gs."); + + assertThat(options.isStatusParallelEnabled()).isFalse(); + } } diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index 270080d87..3f2456ad2 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -178,4 +178,28 @@ public void testAwaitTerminationAndIsTerminated() throws Exception { assertThat(executorService.awaitTermination(10, java.util.concurrent.TimeUnit.SECONDS)) .isTrue(); } + + @Test + public void testExecuteThrowsRejectedExecutionException() { + assertThrows( + java.util.concurrent.RejectedExecutionException.class, + () -> executorService.execute(() -> {})); + } + + @Test + public void testCompletedTaskReturnsResultAfterShutdown() throws Exception { + Callable task = () -> "success"; + Future future = executorService.submit(task); + + // Trigger execution + String result = future.get(); + assertThat(result).isEqualTo("success"); + + // Shut down the executor + executorService.shutdown(); + + // Subsequent calls should still return the result, not throw CancellationException + assertThat(future.get()).isEqualTo("success"); + assertThat(future.get(10, java.util.concurrent.TimeUnit.SECONDS)).isEqualTo("success"); + } } From 6670498e21d479b0ecc0186346050d03a4f2cf64 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 7 Jul 2026 11:35:28 +0000 Subject: [PATCH 22/28] refactor: encapsulate FutureTask logic into LazyFutureTask and update GcsFileSystemImpl instantiation --- .../client/LazyExecutorService.java | 69 ++++----- .../client/LazyExecutorServiceTest.java | 134 ++++++++---------- 2 files changed, 87 insertions(+), 116 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index a45dd75a4..f051ff7f9 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -20,7 +20,6 @@ import java.util.List; import java.util.concurrent.AbstractExecutorService; import java.util.concurrent.Callable; -import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -88,59 +87,39 @@ public Future submit(Callable task) { return newTaskFor(task); } - @Override - protected RunnableFuture newTaskFor(Callable callable) { - return new FutureTask(callable) { - @Override - public T get() throws InterruptedException, ExecutionException { - if (!isDone() && !isCancelled()) { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } + private final class LazyFutureTask extends FutureTask { + LazyFutureTask(Callable callable) { + super(callable); + } + + @Override + public V get() throws InterruptedException, ExecutionException { + if (!isDone() && !isCancelled()) { + if (isShutdown) { + cancel(false); + } else { run(); // Execute on the caller's thread when get() is called } - return super.get(); } + return super.get(); + } - @Override - public T get(long timeout, TimeUnit unit) - throws InterruptedException, ExecutionException, TimeoutException { - if (!isDone() && !isCancelled()) { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } + @Override + public V get(long timeout, TimeUnit unit) + throws InterruptedException, ExecutionException, TimeoutException { + if (!isDone() && !isCancelled()) { + if (isShutdown) { + cancel(false); + } else { run(); } - return super.get(timeout, unit); } - }; + return super.get(timeout, unit); + } } @Override - protected RunnableFuture newTaskFor(Runnable runnable, T value) { - return new FutureTask(runnable, value) { - @Override - public T get() throws InterruptedException, ExecutionException { - if (!isDone() && !isCancelled()) { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } - run(); - } - return super.get(); - } - - @Override - public T get(long timeout, TimeUnit unit) - throws InterruptedException, ExecutionException, TimeoutException { - if (!isDone() && !isCancelled()) { - if (isShutdown) { - throw new CancellationException("Executor is shut down"); - } - run(); - } - return super.get(timeout, unit); - } - }; + protected RunnableFuture newTaskFor(Callable callable) { + return new LazyFutureTask<>(callable); } } diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index 3f2456ad2..1d78afc74 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -33,150 +33,123 @@ public class LazyExecutorServiceTest { private LazyExecutorService executorService; + private AtomicBoolean executed; @Before public void setUp() { executorService = new LazyExecutorService(); + executed = new AtomicBoolean(false); + } + + private Callable createCallableTask() { + return () -> { + executed.set(true); + return "success"; + }; + } + + private Runnable createRunnableTask() { + return () -> executed.set(true); } @Test public void testSubmitCallableIsLazyAndRunsOnCallerThread() throws Exception { - AtomicBoolean executed = new AtomicBoolean(false); AtomicReference executionThread = new AtomicReference<>(); - Callable task = () -> { executed.set(true); executionThread.set(Thread.currentThread()); return "success"; }; - Future future = executorService.submit(task); - - // Verify it has not executed yet assertThat(executed.get()).isFalse(); - // Call get() to trigger execution String result = future.get(); - // Verify execution happened and result is correct - assertThat(executed.get()).isTrue(); assertThat(result).isEqualTo("success"); - - // Verify it ran on the caller's thread + assertThat(executed.get()).isTrue(); assertThat(executionThread.get()).isEqualTo(Thread.currentThread()); } @Test public void testSubmitRunnableIsLazy() throws Exception { - AtomicBoolean executed = new AtomicBoolean(false); - - Runnable task = () -> executed.set(true); - - Future future = executorService.submit(task); - - // Verify it has not executed yet + Future future = executorService.submit(createRunnableTask()); assertThat(executed.get()).isFalse(); - // Call get() to trigger execution future.get(); - // Verify execution happened assertThat(executed.get()).isTrue(); } @Test public void testShutdownThrowsCancellationExceptionOnGet() { - AtomicBoolean executed = new AtomicBoolean(false); - Callable task = - () -> { - executed.set(true); - return "success"; - }; - - Future future = executorService.submit(task); - assertThat(executed.get()).isFalse(); - - // Shutdown the executor + Future future = executorService.submit(createCallableTask()); executorService.shutdown(); - assertThat(executorService.isShutdown()).isTrue(); - // Call get() and expect CancellationException + assertThat(executorService.isShutdown()).isTrue(); assertThrows(CancellationException.class, future::get); - - // Verify task was never executed assertThat(executed.get()).isFalse(); + assertThat(future.isCancelled()).isTrue(); + assertThat(future.isDone()).isTrue(); } @Test public void testShutdownNowReturnsEmptyListAndCancelsFutureTaskExecution() { - AtomicBoolean executed = new AtomicBoolean(false); - Callable task = - () -> { - executed.set(true); - return "success"; - }; + Future future = executorService.submit(createCallableTask()); - Future future = executorService.submit(task); + java.util.List unexecutedTasks = executorService.shutdownNow(); - // shutdownNow should return an empty list since we don't track tasks - assertThat(executorService.shutdownNow()).isEmpty(); + assertThat(unexecutedTasks).isEmpty(); assertThat(executorService.isShutdown()).isTrue(); - assertThrows(CancellationException.class, future::get); assertThat(executed.get()).isFalse(); + assertThat(future.isCancelled()).isTrue(); + assertThat(future.isDone()).isTrue(); } @Test public void testSubmitCallableIsLazyWithTimeout() throws Exception { - AtomicBoolean executed = new AtomicBoolean(false); - Callable task = - () -> { - executed.set(true); - return "success"; - }; - Future future = executorService.submit(task); + Future future = executorService.submit(createCallableTask()); assertThat(executed.get()).isFalse(); String result = future.get(10, java.util.concurrent.TimeUnit.SECONDS); - assertThat(executed.get()).isTrue(); + assertThat(result).isEqualTo("success"); + assertThat(executed.get()).isTrue(); } @Test public void testSubmitRunnableIsLazyWithTimeout() throws Exception { - AtomicBoolean executed = new AtomicBoolean(false); - Runnable task = () -> executed.set(true); - Future future = executorService.submit(task); + Future future = executorService.submit(createRunnableTask()); assertThat(executed.get()).isFalse(); future.get(10, java.util.concurrent.TimeUnit.SECONDS); + assertThat(executed.get()).isTrue(); } @Test public void testShutdownThrowsCancellationExceptionOnGetWithTimeout() { - AtomicBoolean executed = new AtomicBoolean(false); - Callable task = - () -> { - executed.set(true); - return "success"; - }; - Future future = executorService.submit(task); + Future future = executorService.submit(createCallableTask()); executorService.shutdown(); assertThrows( CancellationException.class, () -> future.get(10, java.util.concurrent.TimeUnit.SECONDS)); assertThat(executed.get()).isFalse(); + assertThat(future.isCancelled()).isTrue(); + assertThat(future.isDone()).isTrue(); } @Test public void testAwaitTerminationAndIsTerminated() throws Exception { assertThat(executorService.isTerminated()).isFalse(); + executorService.shutdown(); + boolean terminated = + executorService.awaitTermination(10, java.util.concurrent.TimeUnit.SECONDS); + assertThat(executorService.isTerminated()).isTrue(); - assertThat(executorService.awaitTermination(10, java.util.concurrent.TimeUnit.SECONDS)) - .isTrue(); + assertThat(terminated).isTrue(); } @Test @@ -188,18 +161,37 @@ public void testExecuteThrowsRejectedExecutionException() { @Test public void testCompletedTaskReturnsResultAfterShutdown() throws Exception { - Callable task = () -> "success"; - Future future = executorService.submit(task); + Future future = executorService.submit(createCallableTask()); + future.get(); + + executorService.shutdown(); + + assertThat(future.get()).isEqualTo("success"); + assertThat(future.get(10, java.util.concurrent.TimeUnit.SECONDS)).isEqualTo("success"); + } + + @Test + public void testSubmitRunnableWithResult() throws Exception { + Future future = executorService.submit(createRunnableTask(), "success"); + assertThat(executed.get()).isFalse(); - // Trigger execution String result = future.get(); + assertThat(result).isEqualTo("success"); + assertThat(executed.get()).isTrue(); + } - // Shut down the executor + @Test + public void testSubmitNullTaskThrowsNullPointerException() { + assertThrows(NullPointerException.class, () -> executorService.submit((Callable) null)); + } + + @Test + public void testSubmitAfterShutdownThrowsRejectedExecutionException() { executorService.shutdown(); - // Subsequent calls should still return the result, not throw CancellationException - assertThat(future.get()).isEqualTo("success"); - assertThat(future.get(10, java.util.concurrent.TimeUnit.SECONDS)).isEqualTo("success"); + assertThrows( + java.util.concurrent.RejectedExecutionException.class, + () -> executorService.submit(() -> "task")); } } From 8b1caba10d700bde3f5e6e83f683e1e8f6996d54 Mon Sep 17 00:00:00 2001 From: suni72 Date: Thu, 9 Jul 2026 10:41:44 +0000 Subject: [PATCH 23/28] refactor: improve code readability --- .../client/GcsFileSystemImpl.java | 25 ++++++++- .../client/LazyExecutorService.java | 39 ++++++++++++- .../client/LazyExecutorServiceTest.java | 55 +++++++++---------- 3 files changed, 85 insertions(+), 34 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index d0e82e58f..00914db6a 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -45,6 +45,25 @@ public class GcsFileSystemImpl implements GcsFileSystem { + /** + * Status calls (e.g., getting file info) are lightweight. A core pool size of 2 allows basic + * concurrency without significant resource overhead. + */ + private static final int DEFAULT_STATUS_CORE_POOL_SIZE = 2; + + /** + * Using a 30-second keep-alive enables efficient thread reuse during intermittent spikes in + * status requests, while ensuring rapid resource cleanup during periods of inactivity. + */ + private static final int DEFAULT_STATUS_KEEP_ALIVE_SECONDS = 30; + + /** + * Status calls are lightweight and block on I/O. An unbounded maximum pool size, combined with a + * zero-capacity SynchronousQueue, ensures tasks are never queued and new threads are immediately + * allocated to handle concurrent operations. + */ + private static final int DEFAULT_STATUS_MAX_POOL_SIZE = Integer.MAX_VALUE; + private final GcsClient gcsClient; private final GcsFileSystemOptions fileSystemOptions; private final Supplier readExecutorServiceSupplier; @@ -296,9 +315,9 @@ Supplier initializeListExecutionServiceSupplier() { private static ExecutorService createCachedExecutor() { ThreadPoolExecutor service = new ThreadPoolExecutor( - /* corePoolSize= */ 2, - /* maximumPoolSize= */ Integer.MAX_VALUE, - /* keepAliveTime= */ 30, + /* corePoolSize= */ DEFAULT_STATUS_CORE_POOL_SIZE, + /* maximumPoolSize= */ DEFAULT_STATUS_MAX_POOL_SIZE, + /* keepAliveTime= */ DEFAULT_STATUS_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS, new java.util.concurrent.SynchronousQueue<>(), new ThreadFactoryBuilder() diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index f051ff7f9..ea6f84df8 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -30,10 +30,15 @@ import java.util.concurrent.TimeoutException; /** - * A lightweight, lazy ExecutorService that defers task execution until Future.get() is called. - * Execution happens synchronously on the thread that calls get(). + * A lightweight, lazy ExecutorService that defers task execution until {@code Future.get()} is + * called. Execution happens synchronously on the thread that invokes {@code get()}. + * + *

A returned Future represents a pending task. Upon the first invocation of its {@code get()} + * method, the task executes and its result is permanently cached. + * + *

Both this class and the returned Future are thread-safe. */ -public class LazyExecutorService extends AbstractExecutorService { +final class LazyExecutorService extends AbstractExecutorService { private volatile boolean isShutdown = false; @@ -68,6 +73,28 @@ public void execute(Runnable command) { throw new RejectedExecutionException("Use submit instead of execute."); } + @Override + public List> invokeAll(java.util.Collection> tasks) { + throw new UnsupportedOperationException("LazyExecutorService does not support invokeAll"); + } + + @Override + public List> invokeAll( + java.util.Collection> tasks, long timeout, TimeUnit unit) { + throw new UnsupportedOperationException("LazyExecutorService does not support invokeAll"); + } + + @Override + public T invokeAny(java.util.Collection> tasks) { + throw new UnsupportedOperationException("LazyExecutorService does not support invokeAny"); + } + + @Override + public T invokeAny( + java.util.Collection> tasks, long timeout, TimeUnit unit) { + throw new UnsupportedOperationException("LazyExecutorService does not support invokeAny"); + } + @Override public Future submit(Runnable task) { return submit(Executors.callable(task)); @@ -104,6 +131,12 @@ public V get() throws InterruptedException, ExecutionException { return super.get(); } + /** + * Note: Because this implementation executes the task synchronously on the calling thread, + * the provided timeout is inherently ignored during the actual execution of the task. The calling + * thread will remain blocked until {@code run()} completes, at which point the timeout + * logic evaluates. True preemptive timeouts are not supported. + */ @Override public V get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index 1d78afc74..578e4a36c 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -17,26 +17,23 @@ package com.google.cloud.gcs.analyticscore.client; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; -@RunWith(JUnit4.class) -public class LazyExecutorServiceTest { +class LazyExecutorServiceTest { private LazyExecutorService executorService; private AtomicBoolean executed; - @Before - public void setUp() { + @BeforeEach + void setUp() { executorService = new LazyExecutorService(); executed = new AtomicBoolean(false); } @@ -53,7 +50,7 @@ private Runnable createRunnableTask() { } @Test - public void testSubmitCallableIsLazyAndRunsOnCallerThread() throws Exception { + void submitCallable_isLazyAndRunsOnCallerThread() throws Exception { AtomicReference executionThread = new AtomicReference<>(); Callable task = () -> { @@ -61,9 +58,9 @@ public void testSubmitCallableIsLazyAndRunsOnCallerThread() throws Exception { executionThread.set(Thread.currentThread()); return "success"; }; + Future future = executorService.submit(task); assertThat(executed.get()).isFalse(); - String result = future.get(); assertThat(result).isEqualTo("success"); @@ -72,18 +69,19 @@ public void testSubmitCallableIsLazyAndRunsOnCallerThread() throws Exception { } @Test - public void testSubmitRunnableIsLazy() throws Exception { + void submitRunnable_isLazy() throws Exception { Future future = executorService.submit(createRunnableTask()); - assertThat(executed.get()).isFalse(); + assertThat(executed.get()).isFalse(); future.get(); assertThat(executed.get()).isTrue(); } @Test - public void testShutdownThrowsCancellationExceptionOnGet() { + void shutdown_throwsCancellationExceptionOnGet() { Future future = executorService.submit(createCallableTask()); + executorService.shutdown(); assertThat(executorService.isShutdown()).isTrue(); @@ -94,7 +92,7 @@ public void testShutdownThrowsCancellationExceptionOnGet() { } @Test - public void testShutdownNowReturnsEmptyListAndCancelsFutureTaskExecution() { + void shutdownNow_returnsEmptyListAndCancelsFutureTaskExecution() { Future future = executorService.submit(createCallableTask()); java.util.List unexecutedTasks = executorService.shutdownNow(); @@ -108,10 +106,10 @@ public void testShutdownNowReturnsEmptyListAndCancelsFutureTaskExecution() { } @Test - public void testSubmitCallableIsLazyWithTimeout() throws Exception { + void submitCallable_isLazyWithTimeout() throws Exception { Future future = executorService.submit(createCallableTask()); - assertThat(executed.get()).isFalse(); + assertThat(executed.get()).isFalse(); String result = future.get(10, java.util.concurrent.TimeUnit.SECONDS); assertThat(result).isEqualTo("success"); @@ -119,18 +117,19 @@ public void testSubmitCallableIsLazyWithTimeout() throws Exception { } @Test - public void testSubmitRunnableIsLazyWithTimeout() throws Exception { + void submitRunnable_isLazyWithTimeout() throws Exception { Future future = executorService.submit(createRunnableTask()); - assertThat(executed.get()).isFalse(); + assertThat(executed.get()).isFalse(); future.get(10, java.util.concurrent.TimeUnit.SECONDS); assertThat(executed.get()).isTrue(); } @Test - public void testShutdownThrowsCancellationExceptionOnGetWithTimeout() { + void shutdown_throwsCancellationExceptionOnGetWithTimeout() { Future future = executorService.submit(createCallableTask()); + executorService.shutdown(); assertThrows( @@ -141,10 +140,10 @@ public void testShutdownThrowsCancellationExceptionOnGetWithTimeout() { } @Test - public void testAwaitTerminationAndIsTerminated() throws Exception { + void awaitTermination_andIsTerminated() throws Exception { assertThat(executorService.isTerminated()).isFalse(); - executorService.shutdown(); + boolean terminated = executorService.awaitTermination(10, java.util.concurrent.TimeUnit.SECONDS); @@ -153,14 +152,14 @@ public void testAwaitTerminationAndIsTerminated() throws Exception { } @Test - public void testExecuteThrowsRejectedExecutionException() { + void execute_throwsRejectedExecutionException() { assertThrows( java.util.concurrent.RejectedExecutionException.class, () -> executorService.execute(() -> {})); } @Test - public void testCompletedTaskReturnsResultAfterShutdown() throws Exception { + void completedTask_returnsResultAfterShutdown() throws Exception { Future future = executorService.submit(createCallableTask()); future.get(); @@ -171,10 +170,10 @@ public void testCompletedTaskReturnsResultAfterShutdown() throws Exception { } @Test - public void testSubmitRunnableWithResult() throws Exception { + void submitRunnable_withResult() throws Exception { Future future = executorService.submit(createRunnableTask(), "success"); - assertThat(executed.get()).isFalse(); + assertThat(executed.get()).isFalse(); String result = future.get(); assertThat(result).isEqualTo("success"); @@ -182,12 +181,12 @@ public void testSubmitRunnableWithResult() throws Exception { } @Test - public void testSubmitNullTaskThrowsNullPointerException() { + void submitNullTask_throwsNullPointerException() { assertThrows(NullPointerException.class, () -> executorService.submit((Callable) null)); } @Test - public void testSubmitAfterShutdownThrowsRejectedExecutionException() { + void submitAfterShutdown_throwsRejectedExecutionException() { executorService.shutdown(); assertThrows( From 9d5e8b122a3414d56fdf336b57ebb83e56a30acb Mon Sep 17 00:00:00 2001 From: suni72 Date: Thu, 9 Jul 2026 18:58:07 +0000 Subject: [PATCH 24/28] refactor: simplify LazyExecutorService testing and remove unused imports and tests --- .../client/GcsFileSystemImpl.java | 17 +- .../client/LazyExecutorService.java | 15 +- .../client/GcsClientImplTest.java | 15 -- .../client/GcsFileSystemImplTest.java | 3 + .../client/GcsFileSystemOptionsTest.java | 6 +- .../client/LazyExecutorServiceTest.java | 166 +++++++++++------- 6 files changed, 128 insertions(+), 94 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 00914db6a..468a8bc5e 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -57,6 +57,12 @@ public class GcsFileSystemImpl implements GcsFileSystem { */ private static final int DEFAULT_STATUS_KEEP_ALIVE_SECONDS = 30; + /** + * The maximum amount of time in seconds to wait for background thread pools to gracefully + * terminate upon file system closure. + */ + private static final int SHUTDOWN_TIMEOUT_SECONDS = 10; + /** * Status calls are lightweight and block on I/O. An unbounded maximum pool size, combined with a * zero-capacity SynchronousQueue, ensures tasks are never queued and new threads are immediately @@ -242,12 +248,15 @@ public void close() { readExecutorService.shutdown(); listExecutorService.shutdown(); try { - boolean readTerminated = readExecutorService.awaitTermination(10, TimeUnit.SECONDS); - boolean listTerminated = listExecutorService.awaitTermination(10, TimeUnit.SECONDS); - if (!readTerminated) { + // Wait a total of SHUTDOWN_TIMEOUT_SECONDS for both thread pools to terminate. + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(SHUTDOWN_TIMEOUT_SECONDS); + // First, wait for the read executor service to terminate. + if (!readExecutorService.awaitTermination(SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { readExecutorService.shutdownNow(); } - if (!listTerminated) { + // Then, wait for the status executor service to terminate, with the remaining time. + if (!listExecutorService.awaitTermination( + Math.max(0, deadline - System.nanoTime()), TimeUnit.NANOSECONDS)) { listExecutorService.shutdownNow(); } } catch (InterruptedException e) { diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index ea6f84df8..fd6ab6598 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -58,11 +58,13 @@ public boolean isShutdown() { return isShutdown; } + /** Returns true if the executor has been shut down, since there are no asynchronous tasks. */ @Override public boolean isTerminated() { return isShutdown; } + /** Returns true immediately, since there are no asynchronous tasks or threads to await. */ @Override public boolean awaitTermination(long timeout, TimeUnit unit) { return true; @@ -73,6 +75,10 @@ public void execute(Runnable command) { throw new RejectedExecutionException("Use submit instead of execute."); } + /** + * Bulk execution operations (invokeAll, invokeAny) are not supported by this lazy executor. Tasks + * must be explicitly submitted and resolved individually via their returned Futures. + */ @Override public List> invokeAll(java.util.Collection> tasks) { throw new UnsupportedOperationException("LazyExecutorService does not support invokeAll"); @@ -132,10 +138,10 @@ public V get() throws InterruptedException, ExecutionException { } /** - * Note: Because this implementation executes the task synchronously on the calling thread, - * the provided timeout is inherently ignored during the actual execution of the task. The calling - * thread will remain blocked until {@code run()} completes, at which point the timeout - * logic evaluates. True preemptive timeouts are not supported. + * Note: Because this implementation executes the task synchronously on the calling thread, the + * provided timeout is inherently ignored during the actual execution of the task. The calling + * thread will remain blocked until {@code run()} completes, at which point the timeout logic + * evaluates. True preemptive timeouts are not supported. */ @Override public V get(long timeout, TimeUnit unit) @@ -151,6 +157,7 @@ public V get(long timeout, TimeUnit unit) } } + /** Wraps the given callable into a LazyFutureTask that overrides get() to execute the task. */ @Override protected RunnableFuture newTaskFor(Callable callable) { return new LazyFutureTask<>(callable); diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java index 0d9f64aa4..0a229db7f 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsClientImplTest.java @@ -1064,19 +1064,4 @@ protected Storage createStorage(Optional credentials) { assertThat(channel).isInstanceOf(GcsBidiReadChannel.class); } - - @Test - void getGcsItemInfo_storageThrowsStorageException_throwsIOException() { - Storage mockStorage = mock(Storage.class); - GcsClientImpl localGcsClient = createClientWithMockStorage(mockStorage); - GcsItemId itemId = - GcsItemId.builder().setBucketName("test-bucket-name").setObjectName("test-object").build(); - doThrow(new StorageException(500, "Internal Error")) - .when(mockStorage) - .get(any(BlobId.class), any()); - - IOException e = assertThrows(IOException.class, () -> localGcsClient.getGcsItemInfo(itemId)); - - assertThat(e).hasMessageThat().contains("Unable to access blob"); - } } diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 0313ed152..72913c967 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -397,6 +397,8 @@ void close_whenTerminationTimesOut_shutsDownNow() throws InterruptedException { ExecutorService mockListExecutorService = mock(ExecutorService.class); when(mockReadExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) .thenReturn(false); + when(mockListExecutorService.awaitTermination(anyLong(), any(TimeUnit.class))) + .thenReturn(false); GcsFileSystemImpl fileSystemWithMockExecutor = new GcsFileSystemImpl(mockClient, TEST_GCS_FILESYSTEM_OPTIONS) { @Override @@ -417,6 +419,7 @@ Supplier initializeListExecutionServiceSupplier() { inOrder.verify(mockListExecutorService).shutdown(); inOrder.verify(mockReadExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); inOrder.verify(mockReadExecutorService).shutdownNow(); + inOrder.verify(mockListExecutorService).awaitTermination(anyLong(), any(TimeUnit.class)); inOrder.verify(mockListExecutorService).shutdownNow(); inOrder.verify(mockClient).close(); } diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java index 0f6d42157..1514f07e2 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemOptionsTest.java @@ -84,12 +84,12 @@ void createFromOptions_withDefaultProperties_shouldCreateCorrectOptions() { } @Test - void createFromOptions_withStatusParallelEnabledFalse_createsCorrectOptions() { + void createFromOptions_withListParallelEnabledFalse_createsCorrectOptions() { ImmutableMap properties = - ImmutableMap.of("fs.gs.analytics-core.status.parallel.enabled", "false"); + ImmutableMap.of("fs.gs.analytics-core.list.parallel.enabled", "false"); GcsFileSystemOptions options = GcsFileSystemOptions.createFromOptions(properties, "fs.gs."); - assertThat(options.isStatusParallelEnabled()).isFalse(); + assertThat(options.isListParallelEnabled()).isFalse(); } } diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index 578e4a36c..58f328258 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -17,12 +17,17 @@ package com.google.cloud.gcs.analyticscore.client; import static com.google.common.truth.Truth.assertThat; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.junit.jupiter.api.Assertions.assertThrows; +import java.util.Collections; +import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.Future; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -38,102 +43,91 @@ void setUp() { executed = new AtomicBoolean(false); } - private Callable createCallableTask() { - return () -> { - executed.set(true); - return "success"; - }; + private String createCallableTask() { + executed.set(true); + return "success"; } - private Runnable createRunnableTask() { - return () -> executed.set(true); + private void createRunnableTask() { + executed.set(true); } + /** + * Tests that submitting a Callable is lazy (does not execute on submit), executes on the caller's + * thread when get() is invoked, and multiple get() calls only execute the task once (verifying + * the !isDone() check). + */ @Test - void submitCallable_isLazyAndRunsOnCallerThread() throws Exception { + void submitCallable_isLazyAndExecutesOnceOnCallerThread() throws Exception { AtomicReference executionThread = new AtomicReference<>(); + AtomicInteger executionCount = new AtomicInteger(0); Callable task = () -> { - executed.set(true); + executionCount.incrementAndGet(); executionThread.set(Thread.currentThread()); return "success"; }; - Future future = executorService.submit(task); - assertThat(executed.get()).isFalse(); - String result = future.get(); + boolean executedBeforeGet = executionCount.get() > 0; - assertThat(result).isEqualTo("success"); - assertThat(executed.get()).isTrue(); + String result1 = future.get(10, SECONDS); + String result2 = future.get(); + + assertThat(executedBeforeGet).isFalse(); + assertThat(result1).isEqualTo("success"); + assertThat(result2).isEqualTo("success"); + assertThat(executionCount.get()).isEqualTo(1); assertThat(executionThread.get()).isEqualTo(Thread.currentThread()); } + /** + * Tests that submitting a Runnable is lazy, and multiple get() calls execute the task exactly + * once. + */ @Test - void submitRunnable_isLazy() throws Exception { - Future future = executorService.submit(createRunnableTask()); + void submitRunnable_isLazyAndExecutesOnce() throws Exception { + Future future = executorService.submit(this::createRunnableTask); + boolean executedBeforeGet = executed.get(); - assertThat(executed.get()).isFalse(); future.get(); + future.get(10, SECONDS); + assertThat(executedBeforeGet).isFalse(); assertThat(executed.get()).isTrue(); } + /** + * Tests that if the executor is shut down before a task's get() is called, the task is implicitly + * cancelled and get() throws CancellationException. + */ @Test void shutdown_throwsCancellationExceptionOnGet() { - Future future = executorService.submit(createCallableTask()); + Future future = executorService.submit(this::createCallableTask); executorService.shutdown(); assertThat(executorService.isShutdown()).isTrue(); + assertThrows(CancellationException.class, () -> future.get(10, SECONDS)); assertThrows(CancellationException.class, future::get); assertThat(executed.get()).isFalse(); assertThat(future.isCancelled()).isTrue(); assertThat(future.isDone()).isTrue(); } + /** + * Tests that shutdownNow cancels pending futures similarly to shutdown, and returns an empty list + * since tasks are not queued internally. + */ @Test - void shutdownNow_returnsEmptyListAndCancelsFutureTaskExecution() { - Future future = executorService.submit(createCallableTask()); + void shutdownNow_cancelsTasksAndReturnsEmptyList() { + Future future = executorService.submit(this::createCallableTask); - java.util.List unexecutedTasks = executorService.shutdownNow(); + List unexecutedTasks = executorService.shutdownNow(); assertThat(unexecutedTasks).isEmpty(); assertThat(executorService.isShutdown()).isTrue(); assertThrows(CancellationException.class, future::get); - assertThat(executed.get()).isFalse(); - assertThat(future.isCancelled()).isTrue(); - assertThat(future.isDone()).isTrue(); - } - - @Test - void submitCallable_isLazyWithTimeout() throws Exception { - Future future = executorService.submit(createCallableTask()); - - assertThat(executed.get()).isFalse(); - String result = future.get(10, java.util.concurrent.TimeUnit.SECONDS); - - assertThat(result).isEqualTo("success"); - assertThat(executed.get()).isTrue(); - } - - @Test - void submitRunnable_isLazyWithTimeout() throws Exception { - Future future = executorService.submit(createRunnableTask()); - - assertThat(executed.get()).isFalse(); - future.get(10, java.util.concurrent.TimeUnit.SECONDS); - - assertThat(executed.get()).isTrue(); - } - - @Test - void shutdown_throwsCancellationExceptionOnGetWithTimeout() { - Future future = executorService.submit(createCallableTask()); - - executorService.shutdown(); - - assertThrows( - CancellationException.class, () -> future.get(10, java.util.concurrent.TimeUnit.SECONDS)); + assertThrows(CancellationException.class, () -> future.get(10, SECONDS)); assertThat(executed.get()).isFalse(); assertThat(future.isCancelled()).isTrue(); assertThat(future.isDone()).isTrue(); @@ -144,8 +138,7 @@ void awaitTermination_andIsTerminated() throws Exception { assertThat(executorService.isTerminated()).isFalse(); executorService.shutdown(); - boolean terminated = - executorService.awaitTermination(10, java.util.concurrent.TimeUnit.SECONDS); + boolean terminated = executorService.awaitTermination(10, SECONDS); assertThat(executorService.isTerminated()).isTrue(); assertThat(terminated).isTrue(); @@ -153,44 +146,81 @@ void awaitTermination_andIsTerminated() throws Exception { @Test void execute_throwsRejectedExecutionException() { - assertThrows( - java.util.concurrent.RejectedExecutionException.class, - () -> executorService.execute(() -> {})); + assertThrows(RejectedExecutionException.class, () -> executorService.execute(() -> {})); } + /** + * Tests that if a task completes before shutdown, subsequent get() calls still return the + * successful result instead of throwing CancellationException. + */ @Test void completedTask_returnsResultAfterShutdown() throws Exception { - Future future = executorService.submit(createCallableTask()); + Future future = executorService.submit(this::createCallableTask); future.get(); executorService.shutdown(); assertThat(future.get()).isEqualTo("success"); - assertThat(future.get(10, java.util.concurrent.TimeUnit.SECONDS)).isEqualTo("success"); + assertThat(future.get(10, SECONDS)).isEqualTo("success"); } @Test void submitRunnable_withResult() throws Exception { - Future future = executorService.submit(createRunnableTask(), "success"); + Future future = executorService.submit(this::createRunnableTask, "success"); + boolean executedBeforeGet = executed.get(); - assertThat(executed.get()).isFalse(); - String result = future.get(); + String result1 = future.get(); + String result2 = future.get(10, SECONDS); - assertThat(result).isEqualTo("success"); + assertThat(executedBeforeGet).isFalse(); + assertThat(result1).isEqualTo("success"); + assertThat(result2).isEqualTo("success"); assertThat(executed.get()).isTrue(); } @Test void submitNullTask_throwsNullPointerException() { assertThrows(NullPointerException.class, () -> executorService.submit((Callable) null)); + assertThrows(NullPointerException.class, () -> executorService.submit((Runnable) null)); + assertThrows(NullPointerException.class, () -> executorService.submit((Runnable) null, "res")); } @Test void submitAfterShutdown_throwsRejectedExecutionException() { executorService.shutdown(); + assertThrows(RejectedExecutionException.class, () -> executorService.submit(() -> "task")); + assertThrows(RejectedExecutionException.class, () -> executorService.submit(() -> {})); + } + + /** + * Tests that explicitly cancelling a future prevents it from executing, verifying the + * !isCancelled() check inside the get() execution logic. + */ + @Test + void cancel_preventsTaskExecution() { + Future future = executorService.submit(this::createCallableTask); + + future.cancel(true); + + assertThrows(CancellationException.class, future::get); + assertThrows(CancellationException.class, () -> future.get(10, SECONDS)); + assertThat(executed.get()).isFalse(); + } + + @Test + void invokeMethods_throwUnsupportedOperationException() { + assertThrows( + UnsupportedOperationException.class, + () -> executorService.invokeAll(Collections.emptyList())); + assertThrows( + UnsupportedOperationException.class, + () -> executorService.invokeAll(Collections.emptyList(), 10, SECONDS)); + assertThrows( + UnsupportedOperationException.class, + () -> executorService.invokeAny(Collections.emptyList())); assertThrows( - java.util.concurrent.RejectedExecutionException.class, - () -> executorService.submit(() -> "task")); + UnsupportedOperationException.class, + () -> executorService.invokeAny(Collections.emptyList(), 10, SECONDS)); } } From 93d872e5deb2fb6893856905b18ddd2dd3c445f7 Mon Sep 17 00:00:00 2001 From: suni72 Date: Tue, 14 Jul 2026 09:33:59 +0000 Subject: [PATCH 25/28] Revert telemetry tests changes from dir-metadata-ops-2 --- .../telemetry/LoggingOpenTelemetryProviderTest.java | 6 +++--- .../telemetry/LoggingTelemetryReporterTest.java | 6 +++--- .../common/telemetry/OpenTelemetryReporterTest.java | 2 +- .../common/telemetry/TelemetryOptionsTest.java | 12 ++++++------ 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingOpenTelemetryProviderTest.java b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingOpenTelemetryProviderTest.java index f51f5c577..6ceba672f 100644 --- a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingOpenTelemetryProviderTest.java +++ b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingOpenTelemetryProviderTest.java @@ -23,7 +23,7 @@ class LoggingOpenTelemetryProviderTest { @Test - void get_returnsNonNullInstance() { + void testGet_returnsNonNullInstance() { try (LoggingOpenTelemetryProvider provider = new LoggingOpenTelemetryProvider(OpenTelemetryOptions.builder().build())) { OpenTelemetry openTelemetry = provider.getOpenTelemetry(); @@ -33,7 +33,7 @@ void get_returnsNonNullInstance() { } @Test - void get_returnsSameInstanceOnMultipleCalls() { + void testGet_returnsSameInstanceOnMultipleCalls() { try (LoggingOpenTelemetryProvider provider = new LoggingOpenTelemetryProvider(OpenTelemetryOptions.builder().build())) { OpenTelemetry firstCall = provider.getOpenTelemetry(); @@ -44,7 +44,7 @@ void get_returnsSameInstanceOnMultipleCalls() { } @Test - void constructor_withDuration_createsSuccessfully() { + void testConstructor_withDuration_createsSuccessfully() { try (LoggingOpenTelemetryProvider provider = new LoggingOpenTelemetryProvider( OpenTelemetryOptions.builder().setExportIntervalSeconds(30).build())) { diff --git a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingTelemetryReporterTest.java b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingTelemetryReporterTest.java index 9d474c275..85aadc83f 100644 --- a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingTelemetryReporterTest.java +++ b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/LoggingTelemetryReporterTest.java @@ -26,7 +26,7 @@ class LoggingTelemetryReporterTest { @Test - void formatMetrics_singleMetricWithoutAttributes() { + public void testFormatMetrics_singleMetricWithoutAttributes() { try (LoggingTelemetryReporter reporter = new LoggingTelemetryReporter(LoggingTelemetryOptions.builder().build())) { Map metrics = @@ -43,7 +43,7 @@ void formatMetrics_singleMetricWithoutAttributes() { } @Test - void formatMetrics_singleMetricWithAttributes() { + public void testFormatMetrics_singleMetricWithAttributes() { try (LoggingTelemetryReporter reporter = new LoggingTelemetryReporter(LoggingTelemetryOptions.builder().build())) { Map metrics = @@ -64,7 +64,7 @@ void formatMetrics_singleMetricWithAttributes() { } @Test - void formatMetrics_multipleMetrics() { + public void testFormatMetrics_multipleMetrics() { try (LoggingTelemetryReporter reporter = new LoggingTelemetryReporter(LoggingTelemetryOptions.builder().build())) { Map metrics = diff --git a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/OpenTelemetryReporterTest.java b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/OpenTelemetryReporterTest.java index b0cdcd23d..e652c41d6 100644 --- a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/OpenTelemetryReporterTest.java +++ b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/OpenTelemetryReporterTest.java @@ -67,7 +67,7 @@ void setUp() { } @Test - void operationEnd_recordsMetrics() { + void testOperationEnd_recordsMetrics() { OpenTelemetryOptions options = OpenTelemetryOptions.builder() .setEnabled(true) diff --git a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/TelemetryOptionsTest.java b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/TelemetryOptionsTest.java index 0b2b8b2a6..60c4d32e5 100644 --- a/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/TelemetryOptionsTest.java +++ b/common/src/test/java/com/google/cloud/gcs/analyticscore/common/telemetry/TelemetryOptionsTest.java @@ -22,10 +22,10 @@ import java.util.Map; import org.junit.jupiter.api.Test; -class TelemetryOptionsTest { +public class TelemetryOptionsTest { @Test - void builderWithCustomTelemetryOptions() { + public void testBuilderWithCustomTelemetryOptions() { OperationListener listener = new OperationListener() { @Override @@ -44,7 +44,7 @@ public void onOperationEnd(Operation operation, java.util.Map m } @Test - void createFromOptions_Empty() { + public void testCreateFromOptions_Empty() { Map optionsMap = new HashMap<>(); TelemetryOptions options = TelemetryOptions.createFromOptions(optionsMap, "prefix."); @@ -53,7 +53,7 @@ void createFromOptions_Empty() { } @Test - void createFromOptions_WithLogging() { + public void testCreateFromOptions_WithLogging() { Map optionsMap = new HashMap<>(); optionsMap.put("prefix.telemetry.logging.enabled", "true"); optionsMap.put("prefix.telemetry.logging.level", "INFO"); @@ -68,7 +68,7 @@ void createFromOptions_WithLogging() { } @Test - void createFromOptions_WithOpenTelemetry() { + public void testCreateFromOptions_WithOpenTelemetry() { Map optionsMap = new HashMap<>(); optionsMap.put("prefix.telemetry.opentelemetry.enabled", "true"); optionsMap.put("prefix.telemetry.opentelemetry.provider-type", "LOGGING"); @@ -83,7 +83,7 @@ void createFromOptions_WithOpenTelemetry() { } @Test - void createFromOptions_WithAll() { + public void testCreateFromOptions_WithAll() { Map optionsMap = new HashMap<>(); optionsMap.put("prefix.telemetry.logging.enabled", "true"); optionsMap.put("prefix.telemetry.opentelemetry.enabled", "false"); From 3ac4453f01e3fe68a45b42171d35b5b5f310c658 Mon Sep 17 00:00:00 2001 From: suni72 Date: Mon, 3 Aug 2026 21:12:04 +0000 Subject: [PATCH 26/28] refactor: remove executor service dependency from FlatNamespaceStrategy and update list executor constants and LazyExecutorService termination behavior --- .../client/FlatNamespaceStrategyImpl.java | 8 +-- .../client/GcsFileSystemImpl.java | 53 +++++++++---------- .../client/LazyExecutorService.java | 4 +- .../client/GcsFileSystemImplTest.java | 4 ++ .../client/LazyExecutorServiceTest.java | 2 + 5 files changed, 34 insertions(+), 37 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java index 6e9102b20..11c8b6123 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/FlatNamespaceStrategyImpl.java @@ -16,17 +16,11 @@ package com.google.cloud.gcs.analyticscore.client; -import com.google.common.base.Supplier; -import java.util.concurrent.ExecutorService; - final class FlatNamespaceStrategyImpl implements NamespaceStrategy { private final GcsClient gcsClient; - private final Supplier listExecutorServiceSupplier; - FlatNamespaceStrategyImpl( - GcsClient gcsClient, Supplier listExecutorServiceSupplier) { + FlatNamespaceStrategyImpl(GcsClient gcsClient) { this.gcsClient = gcsClient; - this.listExecutorServiceSupplier = listExecutorServiceSupplier; } } diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index 468a8bc5e..f8b7aef25 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -46,38 +46,36 @@ public class GcsFileSystemImpl implements GcsFileSystem { /** - * Status calls (e.g., getting file info) are lightweight. A core pool size of 2 allows basic - * concurrency without significant resource overhead. + * Status or list calls (e.g., getting file info or listing a directory) block on I/O. A core pool + * size of 2 allows basic concurrency without significant resource overhead. */ - private static final int DEFAULT_STATUS_CORE_POOL_SIZE = 2; + private static final int CACHED_EXECUTOR_CORE_POOL_SIZE = 2; /** * Using a 30-second keep-alive enables efficient thread reuse during intermittent spikes in - * status requests, while ensuring rapid resource cleanup during periods of inactivity. + * status and list requests, while ensuring rapid resource cleanup during periods of inactivity. */ - private static final int DEFAULT_STATUS_KEEP_ALIVE_SECONDS = 30; + private static final int CACHED_EXECUTOR_KEEP_ALIVE_SECONDS = 30; /** - * The maximum amount of time in seconds to wait for background thread pools to gracefully - * terminate upon file system closure. + * Status and list calls block on I/O. An unbounded maximum pool size, combined with a + * zero-capacity SynchronousQueue, ensures tasks are never queued and new threads are immediately + * allocated to handle concurrent operations. */ - private static final int SHUTDOWN_TIMEOUT_SECONDS = 10; + private static final int CACHED_EXECUTOR_MAX_POOL_SIZE = Integer.MAX_VALUE; /** - * Status calls are lightweight and block on I/O. An unbounded maximum pool size, combined with a - * zero-capacity SynchronousQueue, ensures tasks are never queued and new threads are immediately - * allocated to handle concurrent operations. + * The maximum amount of time in seconds to wait for background thread pools to gracefully + * terminate upon file system closure. */ - private static final int DEFAULT_STATUS_MAX_POOL_SIZE = Integer.MAX_VALUE; + private static final int LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS = 10; private final GcsClient gcsClient; private final GcsFileSystemOptions fileSystemOptions; private final Supplier readExecutorServiceSupplier; private final Supplier listExecutorServiceSupplier; - private final Telemetry telemetry; private final AnalyticsCacheManager cacheManager; - private final FlatNamespaceStrategyImpl flatStrategy; private final HierarchicalNamespaceStrategyImpl hnsStrategy; @@ -97,8 +95,7 @@ public GcsFileSystemImpl(GcsFileSystemOptions fileSystemOptions) { fileSystemOptions.getGcsClientOptions(), readExecutorServiceSupplier, telemetry)); - this.flatStrategy = - new FlatNamespaceStrategyImpl(this.gcsClient, this.listExecutorServiceSupplier); + this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -119,8 +116,7 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste fileSystemOptions.getGcsClientOptions(), readExecutorServiceSupplier, telemetry)); - this.flatStrategy = - new FlatNamespaceStrategyImpl(this.gcsClient, this.listExecutorServiceSupplier); + this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -145,8 +141,7 @@ public GcsFileSystemImpl(Credentials credentials, GcsFileSystemOptions fileSyste this.listExecutorServiceSupplier = initializeListExecutionServiceSupplier(); this.telemetry = telemetry; this.cacheManager = cacheManager; - this.flatStrategy = - new FlatNamespaceStrategyImpl(this.gcsClient, this.listExecutorServiceSupplier); + this.flatStrategy = new FlatNamespaceStrategyImpl(this.gcsClient); this.hnsStrategy = new HierarchicalNamespaceStrategyImpl(this.gcsClient); } @@ -248,13 +243,15 @@ public void close() { readExecutorService.shutdown(); listExecutorService.shutdown(); try { - // Wait a total of SHUTDOWN_TIMEOUT_SECONDS for both thread pools to terminate. - long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(SHUTDOWN_TIMEOUT_SECONDS); + // Wait a total of LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS for both thread pools to terminate. + long deadline = + System.nanoTime() + TimeUnit.SECONDS.toNanos(LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS); // First, wait for the read executor service to terminate. - if (!readExecutorService.awaitTermination(SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + if (!readExecutorService.awaitTermination( + LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { readExecutorService.shutdownNow(); } - // Then, wait for the status executor service to terminate, with the remaining time. + // Then, wait for the cached executor service to terminate, with the remaining time. if (!listExecutorService.awaitTermination( Math.max(0, deadline - System.nanoTime()), TimeUnit.NANOSECONDS)) { listExecutorService.shutdownNow(); @@ -324,13 +321,13 @@ Supplier initializeListExecutionServiceSupplier() { private static ExecutorService createCachedExecutor() { ThreadPoolExecutor service = new ThreadPoolExecutor( - /* corePoolSize= */ DEFAULT_STATUS_CORE_POOL_SIZE, - /* maximumPoolSize= */ DEFAULT_STATUS_MAX_POOL_SIZE, - /* keepAliveTime= */ DEFAULT_STATUS_KEEP_ALIVE_SECONDS, + /* corePoolSize= */ CACHED_EXECUTOR_CORE_POOL_SIZE, + /* maximumPoolSize= */ CACHED_EXECUTOR_MAX_POOL_SIZE, + /* keepAliveTime= */ CACHED_EXECUTOR_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS, new java.util.concurrent.SynchronousQueue<>(), new ThreadFactoryBuilder() - .setNameFormat("gcs-filesystem-list-pool-%d") + .setNameFormat("gcs-filesystem-cached-pool-%d") .setDaemon(true) .build()); // allowCoreThreadTimeOut needs to be enabled for cases where the encapsulating class does not diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index fd6ab6598..2a170d81c 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -64,10 +64,10 @@ public boolean isTerminated() { return isShutdown; } - /** Returns true immediately, since there are no asynchronous tasks or threads to await. */ + /** Returns whether the executor has been shut down. */ @Override public boolean awaitTermination(long timeout, TimeUnit unit) { - return true; + return isShutdown; } @Override diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java index 72913c967..b6bfcf7a4 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImplTest.java @@ -327,6 +327,8 @@ void initializeReadExecutionServiceSupplier_shouldReturnMemoizedExecutorService( assertThat(readExecutorServiceSupplier.get()).isInstanceOf(ThreadPoolExecutor.class); assertThat(((ThreadPoolExecutor) readExecutorServiceSupplier.get()).getCorePoolSize()) .isEqualTo(16); + assertThat(readExecutorServiceSupplier.get()) + .isSameInstanceAs(readExecutorServiceSupplier.get()); } @Test @@ -340,6 +342,8 @@ void initializeReadExecutionServiceSupplier_shouldReturnMemoizedExecutorService( assertThat(listExecutorServiceSupplier).isNotNull(); assertThat(listExecutorServiceSupplier.get()).isNotNull(); assertThat(listExecutorServiceSupplier.get()).isInstanceOf(ThreadPoolExecutor.class); + assertThat(listExecutorServiceSupplier.get()) + .isSameInstanceAs(listExecutorServiceSupplier.get()); } @Test diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index 58f328258..f908d1fd6 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -136,6 +136,8 @@ void shutdownNow_cancelsTasksAndReturnsEmptyList() { @Test void awaitTermination_andIsTerminated() throws Exception { assertThat(executorService.isTerminated()).isFalse(); + assertThat(executorService.awaitTermination(10, SECONDS)).isFalse(); + executorService.shutdown(); boolean terminated = executorService.awaitTermination(10, SECONDS); From f289813e6a501df6c1c059f43bc7121dd184f4bf Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 5 Aug 2026 17:09:35 +0000 Subject: [PATCH 27/28] fix: handle thread interruption in LazyExecutorService get methods to prevent task execution when interrupted --- .../client/LazyExecutorService.java | 10 +++++-- .../client/LazyExecutorServiceTest.java | 28 +++++++++++++++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index 2a170d81c..d40ef5b2b 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -127,7 +127,10 @@ private final class LazyFutureTask extends FutureTask { @Override public V get() throws InterruptedException, ExecutionException { - if (!isDone() && !isCancelled()) { + if (!isDone()) { + if (Thread.interrupted()) { + throw new InterruptedException(); + } if (isShutdown) { cancel(false); } else { @@ -146,7 +149,10 @@ public V get() throws InterruptedException, ExecutionException { @Override public V get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - if (!isDone() && !isCancelled()) { + if (!isDone()) { + if (Thread.interrupted()) { + throw new InterruptedException(); + } if (isShutdown) { cancel(false); } else { diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index f908d1fd6..d9ecf93d4 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -196,8 +196,8 @@ void submitAfterShutdown_throwsRejectedExecutionException() { } /** - * Tests that explicitly cancelling a future prevents it from executing, verifying the - * !isCancelled() check inside the get() execution logic. + * Tests that explicitly cancelling a future prevents it from executing, which is natively handled + * by FutureTask's state checks when run() is invoked. */ @Test void cancel_preventsTaskExecution() { @@ -210,6 +210,30 @@ void cancel_preventsTaskExecution() { assertThat(executed.get()).isFalse(); } + @Test + void get_whenThreadInterrupted_throwsInterruptedException() { + Future future = executorService.submit(this::createCallableTask); + Thread.currentThread().interrupt(); + + assertThrows(InterruptedException.class, future::get); + + // Thread.interrupted() clears the interrupt status. + assertThat(Thread.interrupted()).isFalse(); + assertThat(executed.get()).isFalse(); + } + + @Test + void getWithTimeout_whenThreadInterrupted_throwsInterruptedException() { + Future future = executorService.submit(this::createCallableTask); + Thread.currentThread().interrupt(); + + assertThrows(InterruptedException.class, () -> future.get(10, SECONDS)); + + // Thread.interrupted() clears the interrupt status. + assertThat(Thread.interrupted()).isFalse(); + assertThat(executed.get()).isFalse(); + } + @Test void invokeMethods_throwUnsupportedOperationException() { assertThrows( From 2934ad2cd99e31acebb1d888fa54c440042422f1 Mon Sep 17 00:00:00 2001 From: suni72 Date: Wed, 5 Aug 2026 17:55:37 +0000 Subject: [PATCH 28/28] fix: add timeout validation to LazyExecutorService and rename executor shutdown constant in GcsFileSystemImpl --- .../gcs/analyticscore/client/GcsFileSystemImpl.java | 6 +++--- .../gcs/analyticscore/client/LazyExecutorService.java | 8 ++++++++ .../analyticscore/client/LazyExecutorServiceTest.java | 11 +++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java index f8b7aef25..f8aa5d7c8 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/GcsFileSystemImpl.java @@ -68,7 +68,7 @@ public class GcsFileSystemImpl implements GcsFileSystem { * The maximum amount of time in seconds to wait for background thread pools to gracefully * terminate upon file system closure. */ - private static final int LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS = 10; + private static final int EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS = 10; private final GcsClient gcsClient; private final GcsFileSystemOptions fileSystemOptions; @@ -245,10 +245,10 @@ public void close() { try { // Wait a total of LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS for both thread pools to terminate. long deadline = - System.nanoTime() + TimeUnit.SECONDS.toNanos(LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS); + System.nanoTime() + TimeUnit.SECONDS.toNanos(EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS); // First, wait for the read executor service to terminate. if (!readExecutorService.awaitTermination( - LIST_EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { readExecutorService.shutdownNow(); } // Then, wait for the cached executor service to terminate, with the remaining time. diff --git a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java index d40ef5b2b..09c010727 100644 --- a/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java +++ b/client/src/main/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorService.java @@ -36,6 +36,11 @@ *

A returned Future represents a pending task. Upon the first invocation of its {@code get()} * method, the task executes and its result is permanently cached. * + *

Timeout Limitation: Because execution is strictly synchronous on the caller's thread, true + * preemptive timeouts are not supported. If a timeout is provided to {@code get()}, it only checks + * if the timeout has already elapsed prior to execution. If execution begins, it will block + * indefinitely until the task completes, ignoring the timeout duration during execution. + * *

Both this class and the returned Future are thread-safe. */ final class LazyExecutorService extends AbstractExecutorService { @@ -153,6 +158,9 @@ public V get(long timeout, TimeUnit unit) if (Thread.interrupted()) { throw new InterruptedException(); } + if (timeout <= 0) { + throw new TimeoutException(); + } if (isShutdown) { cancel(false); } else { diff --git a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java index d9ecf93d4..99a76ef5d 100644 --- a/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java +++ b/client/src/test/java/com/google/cloud/gcs/analyticscore/client/LazyExecutorServiceTest.java @@ -26,6 +26,7 @@ import java.util.concurrent.CancellationException; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -234,6 +235,16 @@ void getWithTimeout_whenThreadInterrupted_throwsInterruptedException() { assertThat(executed.get()).isFalse(); } + @Test + void getWithTimeout_whenTimeoutZeroOrNegative_throwsTimeoutException() { + Future future = executorService.submit(this::createCallableTask); + + assertThrows(TimeoutException.class, () -> future.get(0, SECONDS)); + assertThrows(TimeoutException.class, () -> future.get(-1, SECONDS)); + + assertThat(executed.get()).isFalse(); + } + @Test void invokeMethods_throwUnsupportedOperationException() { assertThrows(