From 5b4849be3006663fdd826415da1d6d68f2826e0f Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Mon, 11 May 2026 07:33:59 +0000 Subject: [PATCH 01/10] POC for ACO --- java-storage/google-cloud-storage/pom.xml | 1 - .../com/google/cloud/storage/AcoSpan.java | 169 ++++++++++++++++++ .../google/cloud/storage/AcoSpanBuilder.java | 130 ++++++++++++++ .../cloud/storage/BucketMetadataCache.java | 76 ++++++++ .../google/cloud/storage/GrpcStorageImpl.java | 71 ++++++++ .../OtelMultipartUploadClientDecorator.java | 2 +- .../cloud/storage/OtelStorageDecorator.java | 74 +++++++- .../com/google/cloud/storage/StorageImpl.java | 19 ++ .../google/cloud/storage/StorageInternal.java | 8 + .../cloud/storage/spi/v1/HttpStorageRpc.java | 31 ++++ .../cloud/storage/spi/v1/StorageRpc.java | 7 + .../storage/testing/StorageRpcTestBase.java | 5 + .../cloud/storage/ITOpenTelemetryTest.java | 28 ++- .../OtelStorageDecoratorAcoUnitTest.java | 66 +++++++ 14 files changed, 673 insertions(+), 14 deletions(-) create mode 100644 java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java create mode 100644 java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java create mode 100644 java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java create mode 100644 java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java diff --git a/java-storage/google-cloud-storage/pom.xml b/java-storage/google-cloud-storage/pom.xml index 892fd729b9ae..5fcfb43904aa 100644 --- a/java-storage/google-cloud-storage/pom.xml +++ b/java-storage/google-cloud-storage/pom.xml @@ -296,7 +296,6 @@ com.google.api.grpc proto-google-cloud-storage-control-v2 - test com.google.cloud diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java new file mode 100644 index 000000000000..13cfeb8fe2bb --- /dev/null +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java @@ -0,0 +1,169 @@ +/* + * 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.storage; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.api.trace.StatusCode; +import java.util.concurrent.TimeUnit; + +final class AcoSpan implements Span { + private final Span delegate; + private final String bucketName; + private final OtelStorageDecorator parent; + + AcoSpan(Span delegate, String bucketName, OtelStorageDecorator parent) { + this.delegate = delegate; + this.bucketName = bucketName; + this.parent = parent; + } + + private void applyCacheAttributes() { + if (bucketName != null && parent != null && parent.delegate instanceof StorageInternal) { + BucketMetadataCache.BucketMetadata md = + ((StorageInternal) parent.delegate).getBucketMetadataCache().get(bucketName); + if (md != null) { + delegate.setAttribute("gcp.resource.destination.id", md.resource); + delegate.setAttribute("gcp.resource.destination.location", md.location); + } + } + } + + @Override + public void end() { + applyCacheAttributes(); + delegate.end(); + } + + @Override + public void end(long timestamp, TimeUnit unit) { + applyCacheAttributes(); + delegate.end(timestamp, unit); + } + + @Override + public Span recordException(Throwable exception) { + delegate.recordException(exception); + if (exception instanceof StorageException + && parent != null + && parent.delegate instanceof StorageInternal) { + StorageException se = (StorageException) exception; + if (se.getCode() == 404) { + ((StorageInternal) parent.delegate).getBucketMetadataCache().remove(bucketName); + } + } + return this; + } + + @Override + public Span recordException(Throwable exception, Attributes attributes) { + delegate.recordException(exception, attributes); + if (exception instanceof StorageException + && parent != null + && parent.delegate instanceof StorageInternal) { + StorageException se = (StorageException) exception; + if (se.getCode() == 404) { + ((StorageInternal) parent.delegate).getBucketMetadataCache().remove(bucketName); + } + } + return this; + } + + @Override + public Span setAttribute(String k, String v) { + delegate.setAttribute(k, v); + return this; + } + + @Override + public Span setAttribute(String k, long v) { + delegate.setAttribute(k, v); + return this; + } + + @Override + public Span setAttribute(String k, double v) { + delegate.setAttribute(k, v); + return this; + } + + @Override + public Span setAttribute(String k, boolean v) { + delegate.setAttribute(k, v); + return this; + } + + @Override + public Span setAttribute(AttributeKey k, T v) { + delegate.setAttribute(k, v); + return this; + } + + @Override + public Span addEvent(String n) { + delegate.addEvent(n); + return this; + } + + @Override + public Span addEvent(String n, Attributes a) { + delegate.addEvent(n, a); + return this; + } + + @Override + public Span addEvent(String n, long t, TimeUnit u) { + delegate.addEvent(n, t, u); + return this; + } + + @Override + public Span addEvent(String n, Attributes a, long t, TimeUnit u) { + delegate.addEvent(n, a, t, u); + return this; + } + + @Override + public Span setStatus(StatusCode c) { + delegate.setStatus(c); + return this; + } + + @Override + public Span setStatus(StatusCode c, String d) { + delegate.setStatus(c, d); + return this; + } + + @Override + public Span updateName(String name) { + delegate.updateName(name); + return this; + } + + @Override + public SpanContext getSpanContext() { + return delegate.getSpanContext(); + } + + @Override + public boolean isRecording() { + return delegate.isRecording(); + } +} diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java new file mode 100644 index 000000000000..bfcc2a1f77c1 --- /dev/null +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -0,0 +1,130 @@ +/* + * 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.storage; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanBuilder; +import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.context.Context; +import java.util.concurrent.TimeUnit; + +final class AcoSpanBuilder implements SpanBuilder { + private final SpanBuilder delegate; + private final OtelStorageDecorator parent; + private String bucketName; + + AcoSpanBuilder(SpanBuilder delegate, OtelStorageDecorator parent) { + this.delegate = delegate; + this.parent = parent; + } + + @Override + public SpanBuilder setAttribute(String key, String value) { + delegate.setAttribute(key, value); + if ("gsutil.uri".equals(key) && value != null) { + String name = OtelStorageDecorator.extractBucketName(value); + if (name != null && !name.isEmpty()) { + this.bucketName = name; + } + } + return this; + } + + @Override + public SpanBuilder setAttribute(AttributeKey key, T value) { + delegate.setAttribute(key, value); + if ("gsutil.uri".equals(key.getKey()) && value instanceof String) { + String name = OtelStorageDecorator.extractBucketName((String) value); + if (name != null && !name.isEmpty()) { + this.bucketName = name; + } + } + return this; + } + + @Override + public Span startSpan() { + if (bucketName != null && parent != null && parent.delegate instanceof StorageInternal) { + parent.checkCacheAndTriggerFetch(bucketName); + BucketMetadataCache.BucketMetadata md = + ((StorageInternal) parent.delegate).getBucketMetadataCache().get(bucketName); + if (md != null) { + delegate.setAttribute("gcp.resource.destination.id", md.resource); + delegate.setAttribute("gcp.resource.destination.location", md.location); + } + return new AcoSpan(delegate.startSpan(), bucketName, parent); + } + return delegate.startSpan(); + } + + @Override + public SpanBuilder setNoParent() { + delegate.setNoParent(); + return this; + } + + @Override + public SpanBuilder setAttribute(String key, boolean value) { + delegate.setAttribute(key, value); + return this; + } + + @Override + public SpanBuilder setAttribute(String key, double value) { + delegate.setAttribute(key, value); + return this; + } + + @Override + public SpanBuilder setAttribute(String key, long value) { + delegate.setAttribute(key, value); + return this; + } + + @Override + public SpanBuilder setSpanKind(SpanKind k) { + delegate.setSpanKind(k); + return this; + } + + @Override + public SpanBuilder setStartTimestamp(long t, TimeUnit u) { + delegate.setStartTimestamp(t, u); + return this; + } + + @Override + public SpanBuilder setParent(Context c) { + delegate.setParent(c); + return this; + } + + @Override + public SpanBuilder addLink(SpanContext c) { + delegate.addLink(c); + return this; + } + + @Override + public SpanBuilder addLink(SpanContext c, Attributes a) { + delegate.addLink(c, a); + return this; + } +} diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java new file mode 100644 index 000000000000..ef87b6601451 --- /dev/null +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java @@ -0,0 +1,76 @@ +/* + * 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.storage; + +import java.util.LinkedHashMap; +import java.util.Map; + +final class BucketMetadataCache { + + static final class BucketMetadata { + final String resource; + final String location; + + BucketMetadata(String resource, String location) { + this.resource = resource; + this.location = location; + } + } + + private final Object lock = new Object(); + private final Map cache; + + BucketMetadataCache(int capacity) { + this.cache = + new LinkedHashMap(16, 0.75f, true) { + @Override + protected boolean removeEldestEntry(Map.Entry eldest) { + return size() > capacity; + } + }; + } + + BucketMetadata get(String bucketName) { + synchronized (lock) { + return cache.get(bucketName); + } + } + + void put(String bucketName, BucketMetadata metadata) { + synchronized (lock) { + cache.put(bucketName, metadata); + } + } + + void remove(String bucketName) { + synchronized (lock) { + cache.remove(bucketName); + } + } + + void clear() { + synchronized (lock) { + cache.clear(); + } + } + + boolean containsKey(String bucketName) { + synchronized (lock) { + return cache.containsKey(bucketName); + } + } +} diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index 120b7a269724..b670cb2cd714 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -82,6 +82,9 @@ import com.google.iam.v1.GetIamPolicyRequest; import com.google.iam.v1.SetIamPolicyRequest; import com.google.iam.v1.TestIamPermissionsRequest; +import com.google.storage.control.v2.GetStorageLayoutRequest; +import com.google.storage.control.v2.StorageLayout; +import com.google.storage.control.v2.StorageLayoutName; import com.google.storage.v2.AppendObjectSpec; import com.google.storage.v2.BidiReadObjectRequest; import com.google.storage.v2.BidiReadObjectSpec; @@ -112,6 +115,8 @@ import com.google.storage.v2.WriteObjectRequest; import com.google.storage.v2.WriteObjectResponse; import com.google.storage.v2.WriteObjectSpec; +import io.grpc.protobuf.ProtoUtils; +import io.grpc.stub.ClientCalls; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -180,6 +185,8 @@ final class GrpcStorageImpl extends BaseService // workaround for https://github.com/googleapis/java-storage/issues/1736 private final Opts defaultOpts; @Deprecated private final Supplier defaultProjectId; + private volatile BucketMetadataCache bucketMetadataCache; + private final java.lang.Object cacheInitLock = new java.lang.Object(); GrpcStorageImpl( GrpcStorageOptions options, @@ -202,6 +209,70 @@ final class GrpcStorageImpl extends BaseService this.defaultProjectId = Suppliers.memoize(() -> UnifiedOpts.projectId(options.getProjectId())); } + @Override + public BucketMetadataCache getBucketMetadataCache() { + if (bucketMetadataCache == null) { + synchronized (cacheInitLock) { + if (bucketMetadataCache == null) { + bucketMetadataCache = new BucketMetadataCache(10000); + } + } + } + return bucketMetadataCache; + } + + private static final io.grpc.MethodDescriptor + getStorageLayoutMethod = + io.grpc.MethodDescriptor.newBuilder() + .setType(io.grpc.MethodDescriptor.MethodType.UNARY) + .setFullMethodName("google.storage.control.v2.StorageControl/GetStorageLayout") + .setRequestMarshaller( + ProtoUtils.marshaller(GetStorageLayoutRequest.getDefaultInstance())) + .setResponseMarshaller(ProtoUtils.marshaller(StorageLayout.getDefaultInstance())) + .build(); + + @Override + public com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { + io.grpc.Channel channel = null; + try { + com.google.storage.v2.stub.StorageStub stub = storageClient.getStub(); + + java.lang.reflect.Field bgField = + com.google.storage.v2.stub.GrpcStorageStub.class.getDeclaredField("backgroundResources"); + bgField.setAccessible(true); + java.lang.Object bgAggregation = bgField.get(stub); + + java.lang.reflect.Field listField = + bgAggregation.getClass().getDeclaredField("backgroundResources"); + listField.setAccessible(true); + java.util.List resourcesList = (java.util.List) listField.get(bgAggregation); + + for (java.lang.Object res : resourcesList) { + if (res instanceof com.google.api.gax.grpc.GrpcTransportChannel) { + channel = ((com.google.api.gax.grpc.GrpcTransportChannel) res).getChannel(); + break; + } + } + } catch (Exception ex) { + throw new RuntimeException("Failed to extract gRPC channel", ex); + } + + if (channel == null) { + throw new RuntimeException("gRPC channel not found"); + } + + GetStorageLayoutRequest request = + GetStorageLayoutRequest.newBuilder() + .setName(StorageLayoutName.of(getOptions().getProjectId(), bucketName).toString()) + .build(); + + StorageLayout layout = + ClientCalls.blockingUnaryCall( + channel, getStorageLayoutMethod, io.grpc.CallOptions.DEFAULT, request); + + return com.google.cloud.Tuple.of(layout.getName(), layout.getLocation()); + } + @Override public void close() throws Exception { try (StorageClient s = storageClient; diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java index f5e7080fed75..a3832b2e9529 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java @@ -53,7 +53,7 @@ private OtelMultipartUploadClientDecorator( this.delegate = delegate; this.tracer = OtelStorageDecorator.TracerDecorator.decorate( - null, otel, baseAttributes, MultipartUploadClient.class.getName() + "/"); + null, null, otel, baseAttributes, MultipartUploadClient.class.getName() + "/"); } @Override diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index 291db00ae5d3..c0be8f617819 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -58,6 +58,8 @@ import java.nio.file.Path; import java.util.List; import java.util.Locale; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.function.UnaryOperator; import org.checkerframework.checker.nullness.qual.NonNull; @@ -81,7 +83,54 @@ private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes ba this.otel = otel; this.baseAttributes = baseAttributes; this.tracer = - TracerDecorator.decorate(null, otel, baseAttributes, Storage.class.getName() + "/"); + TracerDecorator.decorate(this, null, otel, baseAttributes, Storage.class.getName() + "/"); + } + + static String extractBucketName(String uri) { + if (uri == null || !uri.startsWith("gs://")) { + return null; + } + String remainder = uri.substring(5); + int firstSlash = remainder.indexOf('/'); + if (firstSlash == -1) { + return remainder; + } + return remainder.substring(0, firstSlash); + } + + private final ExecutorService cacheExecutor = + Executors.newCachedThreadPool( + r -> { + Thread t = new Thread(r); + t.setDaemon(true); + t.setName("gcs-aco-metadata-cache-pool"); + return t; + }); + + void checkCacheAndTriggerFetch(String bucketName) { + if (!(delegate instanceof StorageInternal)) { + return; + } + StorageInternal internal = (StorageInternal) delegate; + BucketMetadataCache cache = internal.getBucketMetadataCache(); + + if (cache.containsKey(bucketName)) { + return; + } + + cache.put( + bucketName, + new BucketMetadataCache.BucketMetadata("projects/_/buckets/" + bucketName, "global")); + + cacheExecutor.submit( + () -> { + try { + com.google.cloud.Tuple layout = + internal.internalGetStorageLayout(bucketName); + cache.put(bucketName, new BucketMetadataCache.BucketMetadata(layout.x(), layout.y())); + } catch (Exception e) { + } + }); } @Override @@ -1423,7 +1472,14 @@ public boolean deleteNotification(String bucket, String notificationId) { @Override public void close() throws Exception { - delegate.close(); + try { + if (delegate instanceof StorageInternal) { + ((StorageInternal) delegate).getBucketMetadataCache().clear(); + } + cacheExecutor.shutdownNow(); + } finally { + delegate.close(); + } } @Override @@ -1562,16 +1618,19 @@ static UnaryOperator retryContextDecorator(OpenTelemetry otel) { } static final class TracerDecorator implements Tracer { + @Nullable private final OtelStorageDecorator parentDecorator; @Nullable private final Context parentContextOverride; private final Tracer delegate; private final Attributes baseAttributes; private final String spanNamePrefix; TracerDecorator( + @Nullable OtelStorageDecorator parentDecorator, @Nullable Context parentContextOverride, Tracer delegate, Attributes baseAttributes, String spanNamePrefix) { + this.parentDecorator = parentDecorator; this.parentContextOverride = parentContextOverride; this.delegate = delegate; this.baseAttributes = baseAttributes; @@ -1579,6 +1638,7 @@ static final class TracerDecorator implements Tracer { } static TracerDecorator decorate( + @Nullable OtelStorageDecorator parentDecorator, @Nullable Context parentContextOverride, OpenTelemetry otel, Attributes baseAttributes, @@ -1588,7 +1648,8 @@ static TracerDecorator decorate( requireNonNull(spanNamePrefix, "spanNamePrefix must be non null"); Tracer tracer = otel.getTracer(OTEL_SCOPE_NAME, StorageOptions.getDefaultInstance().getLibraryVersion()); - return new TracerDecorator(parentContextOverride, tracer, baseAttributes, spanNamePrefix); + return new TracerDecorator( + parentDecorator, parentContextOverride, tracer, baseAttributes, spanNamePrefix); } @Override @@ -1598,7 +1659,8 @@ public SpanBuilder spanBuilder(String spanName) { if (parentContextOverride != null) { spanBuilder.setParent(parentContextOverride); } - return spanBuilder; + + return new AcoSpanBuilder(spanBuilder, parentDecorator); } } @@ -1671,6 +1733,7 @@ public OtelDecoratedBlobWriteSession(BlobWriteSession delegate, Span sessionSpan this.sessionSpan = sessionSpan; this.tracer = TracerDecorator.decorate( + OtelStorageDecorator.this, Context.current(), otel, OtelStorageDecorator.this.baseAttributes, @@ -1794,6 +1857,7 @@ public OtelDecoratedCopyWriter(CopyWriter copyWriter, Span span) { this.parentContext = Context.current(); this.tracer = TracerDecorator.decorate( + OtelStorageDecorator.this, Context.current(), otel, OtelStorageDecorator.this.baseAttributes, @@ -2127,6 +2191,7 @@ private OtelDecoratingBlobAppendableUpload(BlobAppendableUpload delegate, Span u this.uploadSpan = uploadSpan; this.tracer = TracerDecorator.decorate( + OtelStorageDecorator.this, Context.current(), otel, OtelStorageDecorator.this.baseAttributes, @@ -2163,6 +2228,7 @@ private OtelDecoratingAppendableUploadWriteableByteChannel( this.openSpan = openSpan; this.tracer = TracerDecorator.decorate( + OtelStorageDecorator.this, Context.current(), otel, OtelStorageDecorator.this.baseAttributes, diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index 8a510b84e7cc..0f1037b05caf 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -117,6 +117,25 @@ final class StorageImpl extends BaseService implements Storage, final StorageRpc storageRpc; final WriterFactory writerFactory; final Retrier retrier; + private volatile BucketMetadataCache bucketMetadataCache; + private final java.lang.Object cacheInitLock = new java.lang.Object(); + + @Override + public BucketMetadataCache getBucketMetadataCache() { + if (bucketMetadataCache == null) { + synchronized (cacheInitLock) { + if (bucketMetadataCache == null) { + bucketMetadataCache = new BucketMetadataCache(10000); + } + } + } + return bucketMetadataCache; + } + + @Override + public com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { + return storageRpc.getStorageLayout(bucketName); + } StorageImpl(HttpStorageOptions options, WriterFactory writerFactory, Retrier retrier) { super(options); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java index 0d700c46df24..e92179eb1596 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java @@ -48,4 +48,12 @@ default BlobInfo compose(ComposeRequest composeRequest) { default BlobInfo internalObjectGet(BlobId blobId, Opts opts) { throw new UnsupportedOperationException("not implemented"); } + + default com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { + throw new UnsupportedOperationException("not implemented"); + } + + default BucketMetadataCache getBucketMetadataCache() { + throw new UnsupportedOperationException("not implemented"); + } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java index 97814b597c37..5db559914c8f 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java @@ -583,6 +583,37 @@ public Bucket get(Bucket bucket, Map options) { } } + @Override + public com.google.cloud.Tuple getStorageLayout(String bucketName) { + try { + String url = options.getHost() + "/storage/v1/b/" + bucketName + "/storageLayout"; + com.google.api.client.http.GenericUrl genericUrl = + new com.google.api.client.http.GenericUrl(url); + com.google.api.client.http.HttpRequest request = + storage.getRequestFactory().buildGetRequest(genericUrl); + com.google.api.client.http.HttpResponse response = request.execute(); + String content = response.parseAsString(); + + String actualResource = "projects/_/buckets/" + bucketName; + String actualLocation = "global"; + + com.google.api.client.json.JsonParser parser = + storage.getJsonFactory().createJsonParser(content); + @SuppressWarnings("unchecked") + Map map = parser.parse(Map.class); + if (map.containsKey("name")) { + actualResource = (String) map.get("name"); + } + if (map.containsKey("location")) { + actualLocation = (String) map.get("location"); + } + + return com.google.cloud.Tuple.of(actualResource, actualLocation); + } catch (IOException e) { + throw translate(e); + } + } + private Storage.Objects.Get getCall(StorageObject object, Map options) throws IOException { Storage.Objects.Get get = storage.objects().get(object.getBucket(), object.getName()); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java index 33f24a54c854..636c8a147792 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java @@ -640,6 +640,13 @@ TestIamPermissionsResponse testIamPermissions( */ ServiceAccount getServiceAccount(String projectId); + /** + * Returns the storage layout for the specified bucket. + * + * @throws StorageException upon failure + */ + com.google.cloud.Tuple getStorageLayout(String bucketName); + @InternalApi Storage getStorage(); } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java index 8f835f5bf3f2..2140e13110e8 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java @@ -322,6 +322,11 @@ public ServiceAccount getServiceAccount(String projectId) { throw new UnsupportedOperationException("Not implemented yet"); } + @Override + public com.google.cloud.Tuple getStorageLayout(String bucketName) { + throw new UnsupportedOperationException("Not implemented yet"); + } + @Override public StorageObject moveObject( String bucket, diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java index 3b8957bbac64..90fe2b6cdd8f 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java @@ -63,20 +63,32 @@ public void checkInstrumentation() throws Exception { storage.getOptions().toBuilder().setOpenTelemetry(openTelemetrySdk).build(); try (Storage storage = storageOptions.getService()) { storage.create(BlobInfo.newBuilder(bucket, generator.randomObjectName()).build()); + Thread.sleep(800); + storage.create(BlobInfo.newBuilder(bucket, generator.randomObjectName()).build()); } - SpanData spanData = exporter.getExportedSpans().get(0); + assertThat(exporter.getExportedSpans().size()).isAtLeast(2); + SpanData span1 = exporter.getExportedSpans().get(0); + SpanData span2 = exporter.getExportedSpans().get(1); + assertAll( - () -> assertThat(getAttributeValue(spanData, "gcp.client.service")).isEqualTo("Storage"), + () -> assertThat(getAttributeValue(span1, "gcp.client.service")).isEqualTo("Storage"), + () -> + assertThat(getAttributeValue(span1, "rpc.system")) + .isEqualTo(transport.name().toLowerCase()), + () -> + assertThat(getAttributeValue(span1, "gcp.resource.destination.id")) + .isEqualTo("projects/_/buckets/" + bucket.getName()), () -> - assertThat(getAttributeValue(spanData, "gcp.client.repo")) - .isEqualTo("googleapis/java-storage"), + assertThat(getAttributeValue(span1, "gcp.resource.destination.location")) + .isEqualTo("global"), + () -> assertThat(getAttributeValue(span2, "gcp.client.service")).isEqualTo("Storage"), () -> - assertThat(getAttributeValue(spanData, "gcp.client.artifact")) - .isEqualTo("com.google.cloud:google-cloud-storage"), + assertThat(getAttributeValue(span2, "gcp.resource.destination.id")) + .contains("buckets/" + bucket.getName()), () -> - assertThat(getAttributeValue(spanData, "rpc.system")) - .isEqualTo(transport.name().toLowerCase())); + assertThat(getAttributeValue(span2, "gcp.resource.destination.location")) + .isNotEqualTo("global")); } @Test diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java new file mode 100644 index 000000000000..c2e27234bd1d --- /dev/null +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java @@ -0,0 +1,66 @@ +/* + * 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.storage; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.mockito.Mockito.mock; + +import io.opentelemetry.api.OpenTelemetry; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanBuilder; +import io.opentelemetry.api.trace.Tracer; +import org.junit.Test; +import org.mockito.Mockito; + +public class OtelStorageDecoratorAcoUnitTest { + + @Test + public void testCacheAndProxyDecorationFlow() throws Exception { + Storage mockStorage = + mock(Storage.class, Mockito.withSettings().extraInterfaces(StorageInternal.class)); + StorageInternal mockInternal = (StorageInternal) mockStorage; + + BucketMetadataCache cache = new BucketMetadataCache(10000); + Mockito.when(mockInternal.getBucketMetadataCache()).thenReturn(cache); + + OpenTelemetry mockOtel = mock(OpenTelemetry.class); + Tracer mockTracer = mock(Tracer.class); + SpanBuilder mockSpanBuilder = mock(SpanBuilder.class); + Span mockSpan = mock(Span.class); + + Mockito.when(mockOtel.getTracer(Mockito.anyString(), Mockito.anyString())) + .thenReturn(mockTracer); + Mockito.when(mockTracer.spanBuilder(Mockito.anyString())).thenReturn(mockSpanBuilder); + Mockito.when(mockSpanBuilder.setAllAttributes(Mockito.any())).thenReturn(mockSpanBuilder); + Mockito.when(mockSpanBuilder.startSpan()).thenReturn(mockSpan); + + Storage decoratedStorage = OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + assertNotNull(decoratedStorage); + + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + osd.checkCacheAndTriggerFetch("test-poc-bucket"); + + BucketMetadataCache.BucketMetadata meta = cache.get("test-poc-bucket"); + assertNotNull(meta); + assertEquals("projects/_/buckets/test-poc-bucket", meta.resource); + assertEquals("global", meta.location); + + cache.remove("test-poc-bucket"); + org.junit.Assert.assertNull(cache.get("test-poc-bucket")); + } +} From f1d1107a99379743c852292318479255c34431da Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Wed, 13 May 2026 03:32:15 +0000 Subject: [PATCH 02/10] enable GetStorageLayout via AcoGrpcStorageStub to replace insecure reflection-based gRPC channel extraction --- .../google/cloud/storage/GrpcStorageImpl.java | 46 ++------- .../cloud/storage/GrpcStorageOptions.java | 99 ++++++++++++++++++- 2 files changed, 104 insertions(+), 41 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index b670cb2cd714..acc8b28b9973 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -221,54 +221,22 @@ public BucketMetadataCache getBucketMetadataCache() { return bucketMetadataCache; } - private static final io.grpc.MethodDescriptor - getStorageLayoutMethod = - io.grpc.MethodDescriptor.newBuilder() - .setType(io.grpc.MethodDescriptor.MethodType.UNARY) - .setFullMethodName("google.storage.control.v2.StorageControl/GetStorageLayout") - .setRequestMarshaller( - ProtoUtils.marshaller(GetStorageLayoutRequest.getDefaultInstance())) - .setResponseMarshaller(ProtoUtils.marshaller(StorageLayout.getDefaultInstance())) - .build(); - @Override public com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { - io.grpc.Channel channel = null; - try { - com.google.storage.v2.stub.StorageStub stub = storageClient.getStub(); - - java.lang.reflect.Field bgField = - com.google.storage.v2.stub.GrpcStorageStub.class.getDeclaredField("backgroundResources"); - bgField.setAccessible(true); - java.lang.Object bgAggregation = bgField.get(stub); - - java.lang.reflect.Field listField = - bgAggregation.getClass().getDeclaredField("backgroundResources"); - listField.setAccessible(true); - java.util.List resourcesList = (java.util.List) listField.get(bgAggregation); - - for (java.lang.Object res : resourcesList) { - if (res instanceof com.google.api.gax.grpc.GrpcTransportChannel) { - channel = ((com.google.api.gax.grpc.GrpcTransportChannel) res).getChannel(); - break; - } - } - } catch (Exception ex) { - throw new RuntimeException("Failed to extract gRPC channel", ex); - } - - if (channel == null) { - throw new RuntimeException("gRPC channel not found"); + com.google.storage.v2.stub.StorageStub rawStub = storageClient.getStub(); + if (!(rawStub instanceof GrpcStorageOptions.AcoGrpcStorageStub)) { + throw new RuntimeException("StorageStub is not an AcoGrpcStorageStub"); } + GrpcStorageOptions.AcoGrpcStorageStub stub = (GrpcStorageOptions.AcoGrpcStorageStub) rawStub; GetStorageLayoutRequest request = GetStorageLayoutRequest.newBuilder() .setName(StorageLayoutName.of(getOptions().getProjectId(), bucketName).toString()) .build(); - StorageLayout layout = - ClientCalls.blockingUnaryCall( - channel, getStorageLayoutMethod, io.grpc.CallOptions.DEFAULT, request); + com.google.api.gax.grpc.GrpcCallContext merge = com.google.cloud.storage.Utils.merge(com.google.api.gax.grpc.GrpcCallContext.createDefault(), Retrying.newCallContext()); + + StorageLayout layout = stub.getStorageLayoutCallable().call(request, merge); return com.google.cloud.Tuple.of(layout.getName(), layout.getLocation()); } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java index 1a6726b9c01b..478926b75cde 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java @@ -45,6 +45,10 @@ import com.google.api.gax.rpc.RequestParamsBuilder; import com.google.api.gax.rpc.RequestParamsExtractor; import com.google.api.gax.rpc.ServerStreamingCallable; +import com.google.api.gax.rpc.UnaryCallable; +import com.google.api.gax.grpc.GrpcCallSettings; +import com.google.storage.control.v2.GetStorageLayoutRequest; +import com.google.storage.control.v2.StorageLayout; import com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials; import com.google.api.gax.tracing.ApiTracerFactory; import com.google.api.pathtemplate.PathTemplate; @@ -951,7 +955,10 @@ public Storage create(StorageOptions options) { } else { LOGGER.config( "zero-copy protobuf deserialization unavailable, proceeding with default"); - StorageClient client = StorageClient.create(storageSettings); + StorageStubSettings baseSettings = (StorageStubSettings) storageSettings.getStubSettings(); + AcoStorageStubSettings.Builder acoStubBuilder = new AcoStorageStubSettings.Builder(baseSettings); + AcoStorageSettings.Builder acoSettingsBuilder = new AcoStorageSettings.Builder(acoStubBuilder); + AcoStorageClient client = new AcoStorageClient(new AcoStorageSettings(acoSettingsBuilder)); StorageDataClient dataClient = StorageDataClient.create( executor, @@ -1103,6 +1110,94 @@ public InternalStorageSettings build() throws IOException { } } + static class AcoGrpcStorageStub extends GrpcStorageStub { + private final UnaryCallable getStorageLayoutCallable; + + AcoGrpcStorageStub( + StorageStubSettings settings, + ClientContext clientContext, + GrpcStubCallableFactory callableFactory) + throws IOException { + super(settings, clientContext, callableFactory); + + MethodDescriptor getStorageLayoutMethod = + MethodDescriptor.newBuilder() + .setType(MethodDescriptor.MethodType.UNARY) + .setFullMethodName("google.storage.control.v2.StorageControl/GetStorageLayout") + .setRequestMarshaller(ProtoUtils.marshaller(GetStorageLayoutRequest.getDefaultInstance())) + .setResponseMarshaller(ProtoUtils.marshaller(StorageLayout.getDefaultInstance())) + .build(); + + GrpcCallSettings transportSettings = + GrpcCallSettings.newBuilder() + .setMethodDescriptor(getStorageLayoutMethod) + .setParamsExtractor(request -> ImmutableMap.of()) + .build(); + + this.getStorageLayoutCallable = + callableFactory.createUnaryCallable( + transportSettings, + com.google.api.gax.rpc.UnaryCallSettings.newUnaryCallSettingsBuilder().build(), + clientContext); + } + + public UnaryCallable getStorageLayoutCallable() { + return getStorageLayoutCallable; + } + } + + private static final class AcoStorageStubSettings extends StorageStubSettings { + private AcoStorageStubSettings(Builder settingsBuilder) throws IOException { + super(settingsBuilder); + } + @Override + public StorageStub createStub() throws IOException { + if (!getTransportChannelProvider() + .getTransportName() + .equals(GrpcTransportChannel.getGrpcTransportName())) { + throw new UnsupportedOperationException( + String.format( + "Transport not supported: %s", getTransportChannelProvider().getTransportName())); + } + ClientContext clientContext = ClientContext.create(this); + return new AcoGrpcStorageStub(this, clientContext, new GrpcStorageCallableFactory()); + } + private static final class Builder extends StorageStubSettings.Builder { + private Builder(StorageStubSettings settings) { + super(settings); + } + @Override + public AcoStorageStubSettings build() throws IOException { + return new AcoStorageStubSettings(this); + } + } + } + + private static final class AcoStorageSettings extends StorageSettings { + private AcoStorageSettings(Builder settingsBuilder) throws IOException { + super(settingsBuilder); + } + private static final class Builder extends StorageSettings.Builder { + private Builder(StorageStubSettings.Builder stubSettings) { + super(stubSettings); + } + @Override + public AcoStorageSettings build() throws IOException { + return new AcoStorageSettings(this); + } + } + } + + private static final class AcoStorageClient extends StorageClient { + private AcoStorageClient(StorageSettings settings) throws IOException { + super(settings); + } + @Override + public AcoGrpcStorageStub getStub() { + return (AcoGrpcStorageStub) super.getStub(); + } + } + private static final class InternalStorageStubSettings extends StorageStubSettings { private InternalStorageStubSettings(Builder settingsBuilder) throws IOException { @@ -1141,7 +1236,7 @@ public InternalStorageStubSettings build() throws IOException { // DanglingJavadocs are for breadcrumbs to source of copied generated code @SuppressWarnings("DanglingJavadoc") - private static final class InternalZeroCopyGrpcStorageStub extends GrpcStorageStub + private static final class InternalZeroCopyGrpcStorageStub extends AcoGrpcStorageStub implements AutoCloseable { private static final RequestParamsExtractor From 49e008c53b2c34898d60cbb5bae85a9769aae91a Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Thu, 14 May 2026 07:30:12 +0000 Subject: [PATCH 03/10] use getBucket instead of getStorageLayout api --- .../google/cloud/storage/GrpcStorageImpl.java | 33 ++++--- .../cloud/storage/GrpcStorageOptions.java | 99 +------------------ .../cloud/storage/OtelStorageDecorator.java | 4 +- .../com/google/cloud/storage/StorageImpl.java | 4 +- .../google/cloud/storage/StorageInternal.java | 2 +- .../cloud/storage/spi/v1/HttpStorageRpc.java | 29 ++---- .../cloud/storage/spi/v1/StorageRpc.java | 4 +- .../storage/testing/StorageRpcTestBase.java | 2 +- 8 files changed, 35 insertions(+), 142 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index acc8b28b9973..16d6bbc074d3 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -96,6 +96,7 @@ import com.google.storage.v2.DeleteBucketRequest; import com.google.storage.v2.DeleteObjectRequest; import com.google.storage.v2.GetBucketRequest; +import com.google.storage.v2.Bucket; import com.google.storage.v2.GetObjectRequest; import com.google.storage.v2.ListBucketsRequest; import com.google.storage.v2.ListObjectsRequest; @@ -222,23 +223,23 @@ public BucketMetadataCache getBucketMetadataCache() { } @Override - public com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { - com.google.storage.v2.stub.StorageStub rawStub = storageClient.getStub(); - if (!(rawStub instanceof GrpcStorageOptions.AcoGrpcStorageStub)) { - throw new RuntimeException("StorageStub is not an AcoGrpcStorageStub"); - } - GrpcStorageOptions.AcoGrpcStorageStub stub = (GrpcStorageOptions.AcoGrpcStorageStub) rawStub; - - GetStorageLayoutRequest request = - GetStorageLayoutRequest.newBuilder() - .setName(StorageLayoutName.of(getOptions().getProjectId(), bucketName).toString()) + public com.google.cloud.Tuple internalGetBucket(String bucketName) { + GetBucketRequest request = + GetBucketRequest.newBuilder() + .setName(bucketNameCodec.encode(bucketName)) .build(); - - com.google.api.gax.grpc.GrpcCallContext merge = com.google.cloud.storage.Utils.merge(com.google.api.gax.grpc.GrpcCallContext.createDefault(), Retrying.newCallContext()); - - StorageLayout layout = stub.getStorageLayoutCallable().call(request, merge); - - return com.google.cloud.Tuple.of(layout.getName(), layout.getLocation()); + GrpcCallContext merge = merge(GrpcCallContext.createDefault(), Retrying.newCallContext()); + Bucket bucket = storageClient.getBucketCallable().call(request, merge); + String pNum = bucket.getProject(); + if (pNum == null || pNum.isEmpty()) { + pNum = "projects/_"; + } + String actualResource = pNum + "/buckets/" + bucketName; + String actualLocation = bucket.getLocation(); + if (actualLocation == null || actualLocation.isEmpty()) { + actualLocation = "global"; + } + return com.google.cloud.Tuple.of(actualResource, actualLocation); } @Override diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java index 478926b75cde..1a6726b9c01b 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java @@ -45,10 +45,6 @@ import com.google.api.gax.rpc.RequestParamsBuilder; import com.google.api.gax.rpc.RequestParamsExtractor; import com.google.api.gax.rpc.ServerStreamingCallable; -import com.google.api.gax.rpc.UnaryCallable; -import com.google.api.gax.grpc.GrpcCallSettings; -import com.google.storage.control.v2.GetStorageLayoutRequest; -import com.google.storage.control.v2.StorageLayout; import com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials; import com.google.api.gax.tracing.ApiTracerFactory; import com.google.api.pathtemplate.PathTemplate; @@ -955,10 +951,7 @@ public Storage create(StorageOptions options) { } else { LOGGER.config( "zero-copy protobuf deserialization unavailable, proceeding with default"); - StorageStubSettings baseSettings = (StorageStubSettings) storageSettings.getStubSettings(); - AcoStorageStubSettings.Builder acoStubBuilder = new AcoStorageStubSettings.Builder(baseSettings); - AcoStorageSettings.Builder acoSettingsBuilder = new AcoStorageSettings.Builder(acoStubBuilder); - AcoStorageClient client = new AcoStorageClient(new AcoStorageSettings(acoSettingsBuilder)); + StorageClient client = StorageClient.create(storageSettings); StorageDataClient dataClient = StorageDataClient.create( executor, @@ -1110,94 +1103,6 @@ public InternalStorageSettings build() throws IOException { } } - static class AcoGrpcStorageStub extends GrpcStorageStub { - private final UnaryCallable getStorageLayoutCallable; - - AcoGrpcStorageStub( - StorageStubSettings settings, - ClientContext clientContext, - GrpcStubCallableFactory callableFactory) - throws IOException { - super(settings, clientContext, callableFactory); - - MethodDescriptor getStorageLayoutMethod = - MethodDescriptor.newBuilder() - .setType(MethodDescriptor.MethodType.UNARY) - .setFullMethodName("google.storage.control.v2.StorageControl/GetStorageLayout") - .setRequestMarshaller(ProtoUtils.marshaller(GetStorageLayoutRequest.getDefaultInstance())) - .setResponseMarshaller(ProtoUtils.marshaller(StorageLayout.getDefaultInstance())) - .build(); - - GrpcCallSettings transportSettings = - GrpcCallSettings.newBuilder() - .setMethodDescriptor(getStorageLayoutMethod) - .setParamsExtractor(request -> ImmutableMap.of()) - .build(); - - this.getStorageLayoutCallable = - callableFactory.createUnaryCallable( - transportSettings, - com.google.api.gax.rpc.UnaryCallSettings.newUnaryCallSettingsBuilder().build(), - clientContext); - } - - public UnaryCallable getStorageLayoutCallable() { - return getStorageLayoutCallable; - } - } - - private static final class AcoStorageStubSettings extends StorageStubSettings { - private AcoStorageStubSettings(Builder settingsBuilder) throws IOException { - super(settingsBuilder); - } - @Override - public StorageStub createStub() throws IOException { - if (!getTransportChannelProvider() - .getTransportName() - .equals(GrpcTransportChannel.getGrpcTransportName())) { - throw new UnsupportedOperationException( - String.format( - "Transport not supported: %s", getTransportChannelProvider().getTransportName())); - } - ClientContext clientContext = ClientContext.create(this); - return new AcoGrpcStorageStub(this, clientContext, new GrpcStorageCallableFactory()); - } - private static final class Builder extends StorageStubSettings.Builder { - private Builder(StorageStubSettings settings) { - super(settings); - } - @Override - public AcoStorageStubSettings build() throws IOException { - return new AcoStorageStubSettings(this); - } - } - } - - private static final class AcoStorageSettings extends StorageSettings { - private AcoStorageSettings(Builder settingsBuilder) throws IOException { - super(settingsBuilder); - } - private static final class Builder extends StorageSettings.Builder { - private Builder(StorageStubSettings.Builder stubSettings) { - super(stubSettings); - } - @Override - public AcoStorageSettings build() throws IOException { - return new AcoStorageSettings(this); - } - } - } - - private static final class AcoStorageClient extends StorageClient { - private AcoStorageClient(StorageSettings settings) throws IOException { - super(settings); - } - @Override - public AcoGrpcStorageStub getStub() { - return (AcoGrpcStorageStub) super.getStub(); - } - } - private static final class InternalStorageStubSettings extends StorageStubSettings { private InternalStorageStubSettings(Builder settingsBuilder) throws IOException { @@ -1236,7 +1141,7 @@ public InternalStorageStubSettings build() throws IOException { // DanglingJavadocs are for breadcrumbs to source of copied generated code @SuppressWarnings("DanglingJavadoc") - private static final class InternalZeroCopyGrpcStorageStub extends AcoGrpcStorageStub + private static final class InternalZeroCopyGrpcStorageStub extends GrpcStorageStub implements AutoCloseable { private static final RequestParamsExtractor diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index c0be8f617819..3d222e572080 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -126,9 +126,11 @@ void checkCacheAndTriggerFetch(String bucketName) { () -> { try { com.google.cloud.Tuple layout = - internal.internalGetStorageLayout(bucketName); + internal.internalGetBucket(bucketName); cache.put(bucketName, new BucketMetadataCache.BucketMetadata(layout.x(), layout.y())); } catch (Exception e) { + System.err.println("Background GetBucket failed: " + e.getMessage()); + e.printStackTrace(); } }); } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index 0f1037b05caf..553fd5632365 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -133,8 +133,8 @@ public BucketMetadataCache getBucketMetadataCache() { } @Override - public com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { - return storageRpc.getStorageLayout(bucketName); + public com.google.cloud.Tuple internalGetBucket(String bucketName) { + return storageRpc.getBucketMetadata(bucketName); } StorageImpl(HttpStorageOptions options, WriterFactory writerFactory, Retrier retrier) { diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java index e92179eb1596..f8e04d64a1aa 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java @@ -49,7 +49,7 @@ default BlobInfo internalObjectGet(BlobId blobId, Opts opts) { throw new UnsupportedOperationException("not implemented"); } - default com.google.cloud.Tuple internalGetStorageLayout(String bucketName) { + default com.google.cloud.Tuple internalGetBucket(String bucketName) { throw new UnsupportedOperationException("not implemented"); } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java index 5db559914c8f..ba21aa4f4930 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java @@ -584,30 +584,15 @@ public Bucket get(Bucket bucket, Map options) { } @Override - public com.google.cloud.Tuple getStorageLayout(String bucketName) { + public com.google.cloud.Tuple getBucketMetadata(String bucketName) { try { - String url = options.getHost() + "/storage/v1/b/" + bucketName + "/storageLayout"; - com.google.api.client.http.GenericUrl genericUrl = - new com.google.api.client.http.GenericUrl(url); - com.google.api.client.http.HttpRequest request = - storage.getRequestFactory().buildGetRequest(genericUrl); - com.google.api.client.http.HttpResponse response = request.execute(); - String content = response.parseAsString(); - - String actualResource = "projects/_/buckets/" + bucketName; - String actualLocation = "global"; - - com.google.api.client.json.JsonParser parser = - storage.getJsonFactory().createJsonParser(content); - @SuppressWarnings("unchecked") - Map map = parser.parse(Map.class); - if (map.containsKey("name")) { - actualResource = (String) map.get("name"); + com.google.api.services.storage.model.Bucket bucket = + storage.buckets().get(bucketName).execute(); + String actualResource = "projects/" + bucket.getProjectNumber() + "/buckets/" + bucketName; + String actualLocation = bucket.getLocation(); + if (actualLocation == null) { + actualLocation = "global"; } - if (map.containsKey("location")) { - actualLocation = (String) map.get("location"); - } - return com.google.cloud.Tuple.of(actualResource, actualLocation); } catch (IOException e) { throw translate(e); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java index 636c8a147792..976f406ab113 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java @@ -641,11 +641,11 @@ TestIamPermissionsResponse testIamPermissions( ServiceAccount getServiceAccount(String projectId); /** - * Returns the storage layout for the specified bucket. + * Returns the bucket resource id and location for the specified bucket. * * @throws StorageException upon failure */ - com.google.cloud.Tuple getStorageLayout(String bucketName); + com.google.cloud.Tuple getBucketMetadata(String bucketName); @InternalApi Storage getStorage(); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java index 2140e13110e8..d24360e0b19d 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java @@ -323,7 +323,7 @@ public ServiceAccount getServiceAccount(String projectId) { } @Override - public com.google.cloud.Tuple getStorageLayout(String bucketName) { + public com.google.cloud.Tuple getBucketMetadata(String bucketName) { throw new UnsupportedOperationException("Not implemented yet"); } From 0b2c48654b7032c2f2748a1e3cd1a96cc1d46553 Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Tue, 19 May 2026 04:02:01 +0000 Subject: [PATCH 04/10] use storage.getBucket and remove cache from storageinternal --- java-storage/google-cloud-storage/pom.xml | 1 + .../com/google/cloud/storage/AcoSpan.java | 16 +++--- .../google/cloud/storage/AcoSpanBuilder.java | 4 +- .../cloud/storage/BucketMetadataCache.java | 37 ++++++++++---- .../google/cloud/storage/GrpcStorageImpl.java | 34 ------------- .../cloud/storage/OtelStorageDecorator.java | 50 ++++++------------- .../com/google/cloud/storage/StorageImpl.java | 19 ------- .../google/cloud/storage/StorageInternal.java | 8 --- .../cloud/storage/spi/v1/HttpStorageRpc.java | 16 ------ .../cloud/storage/spi/v1/StorageRpc.java | 7 --- .../storage/testing/StorageRpcTestBase.java | 5 -- .../OtelStorageDecoratorAcoUnitTest.java | 8 +-- 12 files changed, 54 insertions(+), 151 deletions(-) diff --git a/java-storage/google-cloud-storage/pom.xml b/java-storage/google-cloud-storage/pom.xml index 5fcfb43904aa..892fd729b9ae 100644 --- a/java-storage/google-cloud-storage/pom.xml +++ b/java-storage/google-cloud-storage/pom.xml @@ -296,6 +296,7 @@ com.google.api.grpc proto-google-cloud-storage-control-v2 + test com.google.cloud diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java index 13cfeb8fe2bb..ec71bdd61a7c 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java @@ -35,9 +35,9 @@ final class AcoSpan implements Span { } private void applyCacheAttributes() { - if (bucketName != null && parent != null && parent.delegate instanceof StorageInternal) { + if (bucketName != null && parent != null) { BucketMetadataCache.BucketMetadata md = - ((StorageInternal) parent.delegate).getBucketMetadataCache().get(bucketName); + parent.getBucketMetadataCache().get(bucketName); if (md != null) { delegate.setAttribute("gcp.resource.destination.id", md.resource); delegate.setAttribute("gcp.resource.destination.location", md.location); @@ -60,12 +60,10 @@ public void end(long timestamp, TimeUnit unit) { @Override public Span recordException(Throwable exception) { delegate.recordException(exception); - if (exception instanceof StorageException - && parent != null - && parent.delegate instanceof StorageInternal) { + if (exception instanceof StorageException && parent != null) { StorageException se = (StorageException) exception; if (se.getCode() == 404) { - ((StorageInternal) parent.delegate).getBucketMetadataCache().remove(bucketName); + parent.getBucketMetadataCache().remove(bucketName); } } return this; @@ -74,12 +72,10 @@ public Span recordException(Throwable exception) { @Override public Span recordException(Throwable exception, Attributes attributes) { delegate.recordException(exception, attributes); - if (exception instanceof StorageException - && parent != null - && parent.delegate instanceof StorageInternal) { + if (exception instanceof StorageException && parent != null) { StorageException se = (StorageException) exception; if (se.getCode() == 404) { - ((StorageInternal) parent.delegate).getBucketMetadataCache().remove(bucketName); + parent.getBucketMetadataCache().remove(bucketName); } } return this; diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java index bfcc2a1f77c1..d5d666efc024 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -61,10 +61,10 @@ public SpanBuilder setAttribute(AttributeKey key, T value) { @Override public Span startSpan() { - if (bucketName != null && parent != null && parent.delegate instanceof StorageInternal) { + if (bucketName != null && parent != null) { parent.checkCacheAndTriggerFetch(bucketName); BucketMetadataCache.BucketMetadata md = - ((StorageInternal) parent.delegate).getBucketMetadataCache().get(bucketName); + parent.getBucketMetadataCache().get(bucketName); if (md != null) { delegate.setAttribute("gcp.resource.destination.id", md.resource); delegate.setAttribute("gcp.resource.destination.location", md.location); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java index ef87b6601451..94714be50b88 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java @@ -16,21 +16,13 @@ package com.google.cloud.storage; +import com.google.cloud.Tuple; import java.util.LinkedHashMap; import java.util.Map; final class BucketMetadataCache { - static final class BucketMetadata { - final String resource; - final String location; - - BucketMetadata(String resource, String location) { - this.resource = resource; - this.location = location; - } - } - + private static final int DEFAULT_CAPACITY = 10000; private final Object lock = new Object(); private final Map cache; @@ -44,6 +36,10 @@ protected boolean removeEldestEntry(Map.Entry eldest) { }; } + static BucketMetadataCache getbucketmetadatacache() { + return new BucketMetadataCache(DEFAULT_CAPACITY); + } + BucketMetadata get(String bucketName) { synchronized (lock) { return cache.get(bucketName); @@ -56,6 +52,18 @@ void put(String bucketName, BucketMetadata metadata) { } } + void put(String bucketName, String resource, String location) { + synchronized (lock) { + cache.put(bucketName, new BucketMetadata(resource, location)); + } + } + + void put(String bucketName, Tuple layout) { + synchronized (lock) { + cache.put(bucketName, new BucketMetadata(layout.x(), layout.y())); + } + } + void remove(String bucketName) { synchronized (lock) { cache.remove(bucketName); @@ -73,4 +81,13 @@ boolean containsKey(String bucketName) { return cache.containsKey(bucketName); } } + static final class BucketMetadata { + final String resource; + final String location; + + BucketMetadata(String resource, String location) { + this.resource = resource; + this.location = location; + } + } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index 16d6bbc074d3..f2c2c9b47c6c 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -186,8 +186,6 @@ final class GrpcStorageImpl extends BaseService // workaround for https://github.com/googleapis/java-storage/issues/1736 private final Opts defaultOpts; @Deprecated private final Supplier defaultProjectId; - private volatile BucketMetadataCache bucketMetadataCache; - private final java.lang.Object cacheInitLock = new java.lang.Object(); GrpcStorageImpl( GrpcStorageOptions options, @@ -210,38 +208,6 @@ final class GrpcStorageImpl extends BaseService this.defaultProjectId = Suppliers.memoize(() -> UnifiedOpts.projectId(options.getProjectId())); } - @Override - public BucketMetadataCache getBucketMetadataCache() { - if (bucketMetadataCache == null) { - synchronized (cacheInitLock) { - if (bucketMetadataCache == null) { - bucketMetadataCache = new BucketMetadataCache(10000); - } - } - } - return bucketMetadataCache; - } - - @Override - public com.google.cloud.Tuple internalGetBucket(String bucketName) { - GetBucketRequest request = - GetBucketRequest.newBuilder() - .setName(bucketNameCodec.encode(bucketName)) - .build(); - GrpcCallContext merge = merge(GrpcCallContext.createDefault(), Retrying.newCallContext()); - Bucket bucket = storageClient.getBucketCallable().call(request, merge); - String pNum = bucket.getProject(); - if (pNum == null || pNum.isEmpty()) { - pNum = "projects/_"; - } - String actualResource = pNum + "/buckets/" + bucketName; - String actualLocation = bucket.getLocation(); - if (actualLocation == null || actualLocation.isEmpty()) { - actualLocation = "global"; - } - return com.google.cloud.Tuple.of(actualResource, actualLocation); - } - @Override public void close() throws Exception { try (StorageClient s = storageClient; diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index 3d222e572080..19c601adc601 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -77,6 +77,7 @@ final class OtelStorageDecorator implements Storage { private final OpenTelemetry otel; private final Attributes baseAttributes; private final Tracer tracer; + private final BucketMetadataCache bucketMetadataCache; private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes baseAttributes) { this.delegate = delegate; @@ -84,50 +85,33 @@ private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes ba this.baseAttributes = baseAttributes; this.tracer = TracerDecorator.decorate(this, null, otel, baseAttributes, Storage.class.getName() + "/"); + this.bucketMetadataCache = BucketMetadataCache.getbucketmetadatacache(); } - - static String extractBucketName(String uri) { - if (uri == null || !uri.startsWith("gs://")) { - return null; - } - String remainder = uri.substring(5); - int firstSlash = remainder.indexOf('/'); - if (firstSlash == -1) { - return remainder; - } - return remainder.substring(0, firstSlash); + BucketMetadataCache getBucketMetadataCache() { + return bucketMetadataCache; } - private final ExecutorService cacheExecutor = - Executors.newCachedThreadPool( - r -> { - Thread t = new Thread(r); - t.setDaemon(true); - t.setName("gcs-aco-metadata-cache-pool"); - return t; - }); + Tuple fetch(String bucketName) { + Bucket bucket = delegate.get(bucketName); + System.out.println("getting using delegate.get"); + String actualResource = "projects/" + bucket.getProject() + "/buckets/" + bucketName; + String actualLocation = bucket.getLocation(); + return Tuple.of(actualResource, actualLocation); + } void checkCacheAndTriggerFetch(String bucketName) { - if (!(delegate instanceof StorageInternal)) { + if (bucketMetadataCache.containsKey(bucketName)) { return; } - StorageInternal internal = (StorageInternal) delegate; - BucketMetadataCache cache = internal.getBucketMetadataCache(); - if (cache.containsKey(bucketName)) { - return; - } - - cache.put( - bucketName, - new BucketMetadataCache.BucketMetadata("projects/_/buckets/" + bucketName, "global")); + bucketMetadataCache.put(bucketName, "projects/_/buckets/" + bucketName, "global"); cacheExecutor.submit( () -> { try { com.google.cloud.Tuple layout = - internal.internalGetBucket(bucketName); - cache.put(bucketName, new BucketMetadataCache.BucketMetadata(layout.x(), layout.y())); + fetch(bucketName); + bucketMetadataCache.put(bucketName, layout); } catch (Exception e) { System.err.println("Background GetBucket failed: " + e.getMessage()); e.printStackTrace(); @@ -1475,9 +1459,7 @@ public boolean deleteNotification(String bucket, String notificationId) { @Override public void close() throws Exception { try { - if (delegate instanceof StorageInternal) { - ((StorageInternal) delegate).getBucketMetadataCache().clear(); - } + bucketMetadataCache.clear(); cacheExecutor.shutdownNow(); } finally { delegate.close(); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index 553fd5632365..8a510b84e7cc 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -117,25 +117,6 @@ final class StorageImpl extends BaseService implements Storage, final StorageRpc storageRpc; final WriterFactory writerFactory; final Retrier retrier; - private volatile BucketMetadataCache bucketMetadataCache; - private final java.lang.Object cacheInitLock = new java.lang.Object(); - - @Override - public BucketMetadataCache getBucketMetadataCache() { - if (bucketMetadataCache == null) { - synchronized (cacheInitLock) { - if (bucketMetadataCache == null) { - bucketMetadataCache = new BucketMetadataCache(10000); - } - } - } - return bucketMetadataCache; - } - - @Override - public com.google.cloud.Tuple internalGetBucket(String bucketName) { - return storageRpc.getBucketMetadata(bucketName); - } StorageImpl(HttpStorageOptions options, WriterFactory writerFactory, Retrier retrier) { super(options); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java index f8e04d64a1aa..0d700c46df24 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageInternal.java @@ -48,12 +48,4 @@ default BlobInfo compose(ComposeRequest composeRequest) { default BlobInfo internalObjectGet(BlobId blobId, Opts opts) { throw new UnsupportedOperationException("not implemented"); } - - default com.google.cloud.Tuple internalGetBucket(String bucketName) { - throw new UnsupportedOperationException("not implemented"); - } - - default BucketMetadataCache getBucketMetadataCache() { - throw new UnsupportedOperationException("not implemented"); - } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java index ba21aa4f4930..97814b597c37 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java @@ -583,22 +583,6 @@ public Bucket get(Bucket bucket, Map options) { } } - @Override - public com.google.cloud.Tuple getBucketMetadata(String bucketName) { - try { - com.google.api.services.storage.model.Bucket bucket = - storage.buckets().get(bucketName).execute(); - String actualResource = "projects/" + bucket.getProjectNumber() + "/buckets/" + bucketName; - String actualLocation = bucket.getLocation(); - if (actualLocation == null) { - actualLocation = "global"; - } - return com.google.cloud.Tuple.of(actualResource, actualLocation); - } catch (IOException e) { - throw translate(e); - } - } - private Storage.Objects.Get getCall(StorageObject object, Map options) throws IOException { Storage.Objects.Get get = storage.objects().get(object.getBucket(), object.getName()); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java index 976f406ab113..33f24a54c854 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/StorageRpc.java @@ -640,13 +640,6 @@ TestIamPermissionsResponse testIamPermissions( */ ServiceAccount getServiceAccount(String projectId); - /** - * Returns the bucket resource id and location for the specified bucket. - * - * @throws StorageException upon failure - */ - com.google.cloud.Tuple getBucketMetadata(String bucketName); - @InternalApi Storage getStorage(); } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java index d24360e0b19d..8f835f5bf3f2 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/StorageRpcTestBase.java @@ -322,11 +322,6 @@ public ServiceAccount getServiceAccount(String projectId) { throw new UnsupportedOperationException("Not implemented yet"); } - @Override - public com.google.cloud.Tuple getBucketMetadata(String bucketName) { - throw new UnsupportedOperationException("Not implemented yet"); - } - @Override public StorageObject moveObject( String bucket, diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java index c2e27234bd1d..90dbeee77dae 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java @@ -31,12 +31,7 @@ public class OtelStorageDecoratorAcoUnitTest { @Test public void testCacheAndProxyDecorationFlow() throws Exception { - Storage mockStorage = - mock(Storage.class, Mockito.withSettings().extraInterfaces(StorageInternal.class)); - StorageInternal mockInternal = (StorageInternal) mockStorage; - - BucketMetadataCache cache = new BucketMetadataCache(10000); - Mockito.when(mockInternal.getBucketMetadataCache()).thenReturn(cache); + Storage mockStorage = mock(Storage.class); OpenTelemetry mockOtel = mock(OpenTelemetry.class); Tracer mockTracer = mock(Tracer.class); @@ -55,6 +50,7 @@ public void testCacheAndProxyDecorationFlow() throws Exception { OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; osd.checkCacheAndTriggerFetch("test-poc-bucket"); + BucketMetadataCache cache = osd.getBucketMetadataCache(); BucketMetadataCache.BucketMetadata meta = cache.get("test-poc-bucket"); assertNotNull(meta); assertEquals("projects/_/buckets/test-poc-bucket", meta.resource); From 0e9ba537994f811997e8d242fe95efe8871affef Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Tue, 19 May 2026 05:46:27 +0000 Subject: [PATCH 05/10] correct the location fetched --- .../com/google/cloud/storage/AcoSpan.java | 14 +++-- .../google/cloud/storage/GrpcStorageImpl.java | 6 -- .../cloud/storage/OtelStorageDecorator.java | 57 +++++++++++++++---- 3 files changed, 55 insertions(+), 22 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java index ec71bdd61a7c..3289fd40bc66 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java @@ -62,8 +62,11 @@ public Span recordException(Throwable exception) { delegate.recordException(exception); if (exception instanceof StorageException && parent != null) { StorageException se = (StorageException) exception; - if (se.getCode() == 404) { - parent.getBucketMetadataCache().remove(bucketName); + if (se.getCode() == 404 && se.getMessage() != null) { + String msg = se.getMessage().toLowerCase(java.util.Locale.US); + if (msg.contains("bucket not found") || msg.contains("bucket does not exist")) { + parent.getBucketMetadataCache().remove(bucketName); + } } } return this; @@ -74,8 +77,11 @@ public Span recordException(Throwable exception, Attributes attributes) { delegate.recordException(exception, attributes); if (exception instanceof StorageException && parent != null) { StorageException se = (StorageException) exception; - if (se.getCode() == 404) { - parent.getBucketMetadataCache().remove(bucketName); + if (se.getCode() == 404 && se.getMessage() != null) { + String msg = se.getMessage().toLowerCase(java.util.Locale.US); + if (msg.contains("bucket not found") || msg.contains("bucket does not exist")) { + parent.getBucketMetadataCache().remove(bucketName); + } } } return this; diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index f2c2c9b47c6c..120b7a269724 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -82,9 +82,6 @@ import com.google.iam.v1.GetIamPolicyRequest; import com.google.iam.v1.SetIamPolicyRequest; import com.google.iam.v1.TestIamPermissionsRequest; -import com.google.storage.control.v2.GetStorageLayoutRequest; -import com.google.storage.control.v2.StorageLayout; -import com.google.storage.control.v2.StorageLayoutName; import com.google.storage.v2.AppendObjectSpec; import com.google.storage.v2.BidiReadObjectRequest; import com.google.storage.v2.BidiReadObjectSpec; @@ -96,7 +93,6 @@ import com.google.storage.v2.DeleteBucketRequest; import com.google.storage.v2.DeleteObjectRequest; import com.google.storage.v2.GetBucketRequest; -import com.google.storage.v2.Bucket; import com.google.storage.v2.GetObjectRequest; import com.google.storage.v2.ListBucketsRequest; import com.google.storage.v2.ListObjectsRequest; @@ -116,8 +112,6 @@ import com.google.storage.v2.WriteObjectRequest; import com.google.storage.v2.WriteObjectResponse; import com.google.storage.v2.WriteObjectSpec; -import io.grpc.protobuf.ProtoUtils; -import io.grpc.stub.ClientCalls; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index 19c601adc601..74356ae9020e 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -25,6 +25,7 @@ import com.google.cloud.Policy; import com.google.cloud.ReadChannel; import com.google.cloud.RestorableState; +import com.google.cloud.Tuple; import com.google.cloud.WriteChannel; import com.google.cloud.storage.Acl.Entity; import com.google.cloud.storage.ApiFutureUtils.OnFailureApiFutureCallback; @@ -73,11 +74,15 @@ final class OtelStorageDecorator implements Storage { private static final String BLOB_READ_SESSION = "blobReadSession"; + private static final java.util.logging.Logger LOGGER = + java.util.logging.Logger.getLogger(OtelStorageDecorator.class.getName()); + @VisibleForTesting final Storage delegate; private final OpenTelemetry otel; private final Attributes baseAttributes; private final Tracer tracer; private final BucketMetadataCache bucketMetadataCache; + private final ExecutorService cacheExecutor; private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes baseAttributes) { this.delegate = delegate; @@ -85,18 +90,47 @@ private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes ba this.baseAttributes = baseAttributes; this.tracer = TracerDecorator.decorate(this, null, otel, baseAttributes, Storage.class.getName() + "/"); - this.bucketMetadataCache = BucketMetadataCache.getbucketmetadatacache(); + this.bucketMetadataCache = BucketMetadataCache.getbucketmetadatacache(); + this.cacheExecutor = newCacheExecutor(); + } + + private static ExecutorService newCacheExecutor() { + return Executors.newFixedThreadPool( + 4, + r -> { + Thread t = new Thread(r); + t.setDaemon(true); + t.setName("gcs-aco-metadata-cache-pool"); + return t; + }); } - BucketMetadataCache getBucketMetadataCache() { + + BucketMetadataCache getBucketMetadataCache() { return bucketMetadataCache; } - Tuple fetch(String bucketName) { - Bucket bucket = delegate.get(bucketName); - System.out.println("getting using delegate.get"); - String actualResource = "projects/" + bucket.getProject() + "/buckets/" + bucketName; - String actualLocation = bucket.getLocation(); - return Tuple.of(actualResource, actualLocation); + Tuple fetch(String bucketName) { + Bucket bucket = delegate.get(bucketName); + if (bucket == null) { + return null; + } + + String projectId = bucket.getProject() != null ? bucket.getProject().toString() : null; + String resource; + if (!projectId.isEmpty()) { + resource = "projects/" + projectId + "/buckets/" + bucketName; + } else { + resource = "projects/_/buckets/" + bucketName; + } + + String location = bucket.getLocation() != null ? bucket.getLocation().toLowerCase(Locale.US) : "global"; + String locationType = bucket.getLocationType() != null ? bucket.getLocationType().toLowerCase(Locale.US) : "region"; + + if ("multi-region".equals(locationType) || "dual-region".equals(locationType)) { + location = "global"; + } + + return Tuple.of(resource, location); } void checkCacheAndTriggerFetch(String bucketName) { @@ -109,12 +143,11 @@ void checkCacheAndTriggerFetch(String bucketName) { cacheExecutor.submit( () -> { try { - com.google.cloud.Tuple layout = + Tuple layout = fetch(bucketName); bucketMetadataCache.put(bucketName, layout); } catch (Exception e) { - System.err.println("Background GetBucket failed: " + e.getMessage()); - e.printStackTrace(); + LOGGER.log(java.util.logging.Level.WARNING, "Background GetBucket failed", e); } }); } @@ -1460,7 +1493,7 @@ public boolean deleteNotification(String bucket, String notificationId) { public void close() throws Exception { try { bucketMetadataCache.clear(); - cacheExecutor.shutdownNow(); + cacheExecutor.shutdown(); } finally { delegate.close(); } From ce14022d99aa776586178e20f3d581913c602b05 Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Tue, 19 May 2026 06:11:51 +0000 Subject: [PATCH 06/10] move helper methods to AcoSpanBuilder --- .../com/google/cloud/storage/AcoSpan.java | 6 +- .../google/cloud/storage/AcoSpanBuilder.java | 88 ++++++++++++++++++- .../cloud/storage/OtelStorageDecorator.java | 67 +------------- .../OtelStorageDecoratorAcoUnitTest.java | 4 +- 4 files changed, 92 insertions(+), 73 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java index 3289fd40bc66..fbcb2d591885 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java @@ -37,7 +37,7 @@ final class AcoSpan implements Span { private void applyCacheAttributes() { if (bucketName != null && parent != null) { BucketMetadataCache.BucketMetadata md = - parent.getBucketMetadataCache().get(bucketName); + parent.bucketMetadataCache.get(bucketName); if (md != null) { delegate.setAttribute("gcp.resource.destination.id", md.resource); delegate.setAttribute("gcp.resource.destination.location", md.location); @@ -65,7 +65,7 @@ public Span recordException(Throwable exception) { if (se.getCode() == 404 && se.getMessage() != null) { String msg = se.getMessage().toLowerCase(java.util.Locale.US); if (msg.contains("bucket not found") || msg.contains("bucket does not exist")) { - parent.getBucketMetadataCache().remove(bucketName); + parent.bucketMetadataCache.remove(bucketName); } } } @@ -80,7 +80,7 @@ public Span recordException(Throwable exception, Attributes attributes) { if (se.getCode() == 404 && se.getMessage() != null) { String msg = se.getMessage().toLowerCase(java.util.Locale.US); if (msg.contains("bucket not found") || msg.contains("bucket does not exist")) { - parent.getBucketMetadataCache().remove(bucketName); + parent.bucketMetadataCache.remove(bucketName); } } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java index d5d666efc024..37d696836dd7 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -16,6 +16,7 @@ package com.google.cloud.storage; +import com.google.cloud.Tuple; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.trace.Span; @@ -23,9 +24,17 @@ import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.context.Context; +import java.util.Locale; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.logging.Level; +import java.util.logging.Logger; final class AcoSpanBuilder implements SpanBuilder { + + private static final Logger LOGGER = Logger.getLogger(AcoSpanBuilder.class.getName()); + private final SpanBuilder delegate; private final OtelStorageDecorator parent; private String bucketName; @@ -39,7 +48,7 @@ final class AcoSpanBuilder implements SpanBuilder { public SpanBuilder setAttribute(String key, String value) { delegate.setAttribute(key, value); if ("gsutil.uri".equals(key) && value != null) { - String name = OtelStorageDecorator.extractBucketName(value); + String name = extractBucketName(value); if (name != null && !name.isEmpty()) { this.bucketName = name; } @@ -51,7 +60,7 @@ public SpanBuilder setAttribute(String key, String value) { public SpanBuilder setAttribute(AttributeKey key, T value) { delegate.setAttribute(key, value); if ("gsutil.uri".equals(key.getKey()) && value instanceof String) { - String name = OtelStorageDecorator.extractBucketName((String) value); + String name = extractBucketName((String) value); if (name != null && !name.isEmpty()) { this.bucketName = name; } @@ -62,9 +71,9 @@ public SpanBuilder setAttribute(AttributeKey key, T value) { @Override public Span startSpan() { if (bucketName != null && parent != null) { - parent.checkCacheAndTriggerFetch(bucketName); + checkCacheAndTriggerFetch(parent.delegate, parent.bucketMetadataCache, parent.cacheExecutor, bucketName); BucketMetadataCache.BucketMetadata md = - parent.getBucketMetadataCache().get(bucketName); + parent.bucketMetadataCache.get(bucketName); if (md != null) { delegate.setAttribute("gcp.resource.destination.id", md.resource); delegate.setAttribute("gcp.resource.destination.location", md.location); @@ -127,4 +136,75 @@ public SpanBuilder addLink(SpanContext c, Attributes a) { delegate.addLink(c, a); return this; } + + static ExecutorService newCacheExecutor() { + return Executors.newFixedThreadPool( + 4, + r -> { + Thread t = new Thread(r); + t.setDaemon(true); + t.setName("gcs-aco-metadata-cache-pool"); + return t; + }); + } + + static String extractBucketName(String uri) { + if (uri == null || !uri.startsWith("gs://")) { + return null; + } + String remainder = uri.substring(5); + int firstSlash = remainder.indexOf('/'); + if (firstSlash == -1) { + return remainder; + } + return remainder.substring(0, firstSlash); + } + + static Tuple fetch(Storage delegate, String bucketName) { + Bucket bucket = delegate.get(bucketName); + if (bucket == null) { + return null; + } + + String projectId = bucket.getProject() != null ? bucket.getProject().toString() : null; + String resource; + if (projectId != null && !projectId.isEmpty()) { + resource = "projects/" + projectId + "/buckets/" + bucketName; + } else { + resource = "projects/_/buckets/" + bucketName; + } + + String location = bucket.getLocation() != null ? bucket.getLocation().toLowerCase(Locale.US) : "global"; + String locationType = bucket.getLocationType() != null ? bucket.getLocationType().toLowerCase(Locale.US) : "region"; + + if ("multi-region".equals(locationType) || "dual-region".equals(locationType)) { + location = "global"; + } + + return Tuple.of(resource, location); + } + + static void checkCacheAndTriggerFetch( + Storage delegate, + BucketMetadataCache bucketMetadataCache, + ExecutorService cacheExecutor, + String bucketName) { + if (bucketMetadataCache.containsKey(bucketName)) { + return; + } + + bucketMetadataCache.put(bucketName, "projects/_/buckets/" + bucketName, "global"); + + cacheExecutor.submit( + () -> { + try { + Tuple layout = fetch(delegate, bucketName); + if (layout != null) { + bucketMetadataCache.put(bucketName, layout); + } + } catch (Exception e) { + LOGGER.log(Level.WARNING, "Background GetBucket failed", e); + } + }); + } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index 74356ae9020e..b9281d56427d 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -25,7 +25,6 @@ import com.google.cloud.Policy; import com.google.cloud.ReadChannel; import com.google.cloud.RestorableState; -import com.google.cloud.Tuple; import com.google.cloud.WriteChannel; import com.google.cloud.storage.Acl.Entity; import com.google.cloud.storage.ApiFutureUtils.OnFailureApiFutureCallback; @@ -74,15 +73,12 @@ final class OtelStorageDecorator implements Storage { private static final String BLOB_READ_SESSION = "blobReadSession"; - private static final java.util.logging.Logger LOGGER = - java.util.logging.Logger.getLogger(OtelStorageDecorator.class.getName()); - @VisibleForTesting final Storage delegate; private final OpenTelemetry otel; private final Attributes baseAttributes; private final Tracer tracer; - private final BucketMetadataCache bucketMetadataCache; - private final ExecutorService cacheExecutor; + final BucketMetadataCache bucketMetadataCache; + final ExecutorService cacheExecutor; private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes baseAttributes) { this.delegate = delegate; @@ -91,66 +87,9 @@ private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes ba this.tracer = TracerDecorator.decorate(this, null, otel, baseAttributes, Storage.class.getName() + "/"); this.bucketMetadataCache = BucketMetadataCache.getbucketmetadatacache(); - this.cacheExecutor = newCacheExecutor(); - } - - private static ExecutorService newCacheExecutor() { - return Executors.newFixedThreadPool( - 4, - r -> { - Thread t = new Thread(r); - t.setDaemon(true); - t.setName("gcs-aco-metadata-cache-pool"); - return t; - }); - } - - BucketMetadataCache getBucketMetadataCache() { - return bucketMetadataCache; + this.cacheExecutor = AcoSpanBuilder.newCacheExecutor(); } - Tuple fetch(String bucketName) { - Bucket bucket = delegate.get(bucketName); - if (bucket == null) { - return null; - } - - String projectId = bucket.getProject() != null ? bucket.getProject().toString() : null; - String resource; - if (!projectId.isEmpty()) { - resource = "projects/" + projectId + "/buckets/" + bucketName; - } else { - resource = "projects/_/buckets/" + bucketName; - } - - String location = bucket.getLocation() != null ? bucket.getLocation().toLowerCase(Locale.US) : "global"; - String locationType = bucket.getLocationType() != null ? bucket.getLocationType().toLowerCase(Locale.US) : "region"; - - if ("multi-region".equals(locationType) || "dual-region".equals(locationType)) { - location = "global"; - } - - return Tuple.of(resource, location); - } - - void checkCacheAndTriggerFetch(String bucketName) { - if (bucketMetadataCache.containsKey(bucketName)) { - return; - } - - bucketMetadataCache.put(bucketName, "projects/_/buckets/" + bucketName, "global"); - - cacheExecutor.submit( - () -> { - try { - Tuple layout = - fetch(bucketName); - bucketMetadataCache.put(bucketName, layout); - } catch (Exception e) { - LOGGER.log(java.util.logging.Level.WARNING, "Background GetBucket failed", e); - } - }); - } @Override public Bucket create(BucketInfo bucketInfo, BucketTargetOption... options) { diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java index 90dbeee77dae..fe04abd2c560 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java @@ -48,9 +48,9 @@ public void testCacheAndProxyDecorationFlow() throws Exception { assertNotNull(decoratedStorage); OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - osd.checkCacheAndTriggerFetch("test-poc-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch(osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "test-poc-bucket"); - BucketMetadataCache cache = osd.getBucketMetadataCache(); + BucketMetadataCache cache = osd.bucketMetadataCache; BucketMetadataCache.BucketMetadata meta = cache.get("test-poc-bucket"); assertNotNull(meta); assertEquals("projects/_/buckets/test-poc-bucket", meta.resource); From 8636ad42adf567ddddb00c816b7035875a317e66 Mon Sep 17 00:00:00 2001 From: nidhiii-27 Date: Tue, 19 May 2026 14:59:03 +0530 Subject: [PATCH 07/10] add more extensive tests --- .../com/google/cloud/storage/AcoSpan.java | 5 +- .../google/cloud/storage/AcoSpanBuilder.java | 36 ++-- .../cloud/storage/BucketMetadataCache.java | 15 +- .../cloud/storage/OtelStorageDecorator.java | 3 - .../cloud/storage/ITOpenTelemetryTest.java | 77 +++++++- .../OtelStorageDecoratorAcoUnitTest.java | 182 ++++++++++++++++-- 6 files changed, 277 insertions(+), 41 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java index fbcb2d591885..d2972fab8900 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java @@ -36,9 +36,8 @@ final class AcoSpan implements Span { private void applyCacheAttributes() { if (bucketName != null && parent != null) { - BucketMetadataCache.BucketMetadata md = - parent.bucketMetadataCache.get(bucketName); - if (md != null) { + BucketMetadataCache.BucketMetadata md = parent.bucketMetadataCache.get(bucketName); + if (md != null && !md.fetchPending) { delegate.setAttribute("gcp.resource.destination.id", md.resource); delegate.setAttribute("gcp.resource.destination.location", md.location); } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java index 37d696836dd7..1270a61368af 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -71,13 +71,8 @@ public SpanBuilder setAttribute(AttributeKey key, T value) { @Override public Span startSpan() { if (bucketName != null && parent != null) { - checkCacheAndTriggerFetch(parent.delegate, parent.bucketMetadataCache, parent.cacheExecutor, bucketName); - BucketMetadataCache.BucketMetadata md = - parent.bucketMetadataCache.get(bucketName); - if (md != null) { - delegate.setAttribute("gcp.resource.destination.id", md.resource); - delegate.setAttribute("gcp.resource.destination.location", md.location); - } + checkCacheAndTriggerFetch( + parent.delegate, parent.bucketMetadataCache, parent.cacheExecutor, bucketName); return new AcoSpan(delegate.startSpan(), bucketName, parent); } return delegate.startSpan(); @@ -174,8 +169,12 @@ static Tuple fetch(Storage delegate, String bucketName) { resource = "projects/_/buckets/" + bucketName; } - String location = bucket.getLocation() != null ? bucket.getLocation().toLowerCase(Locale.US) : "global"; - String locationType = bucket.getLocationType() != null ? bucket.getLocationType().toLowerCase(Locale.US) : "region"; + String location = + bucket.getLocation() != null ? bucket.getLocation().toLowerCase(Locale.US) : "global"; + String locationType = + bucket.getLocationType() != null + ? bucket.getLocationType().toLowerCase(Locale.US) + : "region"; if ("multi-region".equals(locationType) || "dual-region".equals(locationType)) { location = "global"; @@ -193,14 +192,29 @@ static void checkCacheAndTriggerFetch( return; } - bucketMetadataCache.put(bucketName, "projects/_/buckets/" + bucketName, "global"); + // Put fetchPending placeholder synchronously to block concurrent stampedes + bucketMetadataCache.put(bucketName, "projects/_/buckets/" + bucketName, "global", true); cacheExecutor.submit( () -> { try { Tuple layout = fetch(delegate, bucketName); if (layout != null) { - bucketMetadataCache.put(bucketName, layout); + bucketMetadataCache.put(bucketName, layout, false); + } else { + // Bucket does not exist (fetch returned null) -> Evict cache entry + bucketMetadataCache.remove(bucketName); + } + } catch (StorageException e) { + if (e.getCode() == 404) { + // Bucket not found -> Evict cache entry + bucketMetadataCache.remove(bucketName); + } else if (e.getCode() == 403) { + // Permission Denied -> Retain fallback values with fetchPending=false (Do Not Retry) + bucketMetadataCache.put( + bucketName, "projects/_/buckets/" + bucketName, "global", false); + } else { + LOGGER.log(Level.WARNING, "Background GetBucket failed", e); } } catch (Exception e) { LOGGER.log(Level.WARNING, "Background GetBucket failed", e); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java index 94714be50b88..c9d48dcfe374 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java @@ -52,15 +52,15 @@ void put(String bucketName, BucketMetadata metadata) { } } - void put(String bucketName, String resource, String location) { + void put(String bucketName, String resource, String location, boolean pending) { synchronized (lock) { - cache.put(bucketName, new BucketMetadata(resource, location)); + cache.put(bucketName, new BucketMetadata(resource, location, pending)); } } - void put(String bucketName, Tuple layout) { + void put(String bucketName, Tuple layout, boolean pending) { synchronized (lock) { - cache.put(bucketName, new BucketMetadata(layout.x(), layout.y())); + cache.put(bucketName, new BucketMetadata(layout.x(), layout.y(), pending)); } } @@ -81,13 +81,16 @@ boolean containsKey(String bucketName) { return cache.containsKey(bucketName); } } - static final class BucketMetadata { + + static final class BucketMetadata { final String resource; final String location; + final boolean fetchPending; - BucketMetadata(String resource, String location) { + BucketMetadata(String resource, String location, boolean fetchPending) { this.resource = resource; this.location = location; + this.fetchPending = fetchPending; } } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index b9281d56427d..eb0edcb52705 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -59,7 +59,6 @@ import java.util.List; import java.util.Locale; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.function.UnaryOperator; import org.checkerframework.checker.nullness.qual.NonNull; @@ -90,7 +89,6 @@ private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes ba this.cacheExecutor = AcoSpanBuilder.newCacheExecutor(); } - @Override public Bucket create(BucketInfo bucketInfo, BucketTargetOption... options) { Span span = @@ -1615,7 +1613,6 @@ public SpanBuilder spanBuilder(String spanName) { if (parentContextOverride != null) { spanBuilder.setParent(parentContextOverride); } - return new AcoSpanBuilder(spanBuilder, parentDecorator); } } diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java index 90fe2b6cdd8f..392cb4349df9 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java @@ -76,12 +76,6 @@ public void checkInstrumentation() throws Exception { () -> assertThat(getAttributeValue(span1, "rpc.system")) .isEqualTo(transport.name().toLowerCase()), - () -> - assertThat(getAttributeValue(span1, "gcp.resource.destination.id")) - .isEqualTo("projects/_/buckets/" + bucket.getName()), - () -> - assertThat(getAttributeValue(span1, "gcp.resource.destination.location")) - .isEqualTo("global"), () -> assertThat(getAttributeValue(span2, "gcp.client.service")).isEqualTo("Storage"), () -> assertThat(getAttributeValue(span2, "gcp.resource.destination.id")) @@ -91,6 +85,77 @@ public void checkInstrumentation() throws Exception { .isNotEqualTo("global")); } + @Test + public void testAcoNonExistentBucketNoAttributes() throws Exception { + TestExporter exporter = new TestExporter(); + OpenTelemetrySdk openTelemetrySdk = + OpenTelemetrySdk.builder() + .setTracerProvider( + SdkTracerProvider.builder() + .addSpanProcessor(SimpleSpanProcessor.create(exporter)) + .build()) + .build(); + StorageOptions storageOptions = + storage.getOptions().toBuilder().setOpenTelemetry(openTelemetrySdk).build(); + String nonExistentBucket = "non-existent-bucket-" + generator.randomBucketName(); + + try (Storage storage = storageOptions.getService()) { + storage.get(nonExistentBucket); + Thread.sleep(800); + storage.get(nonExistentBucket); + } + + // We should have at least 2 get spans + assertThat(exporter.getExportedSpans().size()).isAtLeast(2); + SpanData getSpan1 = exporter.getExportedSpans().get(0); + SpanData getSpan2 = exporter.getExportedSpans().get(1); + + assertAll( + () -> assertThat(getAttributeValue(getSpan2, "gcp.resource.destination.id")).isNull(), + () -> + assertThat(getAttributeValue(getSpan2, "gcp.resource.destination.location")).isNull()); + } + + @Test + public void testAcoForbiddenBucketFallbackAttributes() throws Exception { + TestExporter exporter = new TestExporter(); + OpenTelemetrySdk openTelemetrySdk = + OpenTelemetrySdk.builder() + .setTracerProvider( + SdkTracerProvider.builder() + .addSpanProcessor(SimpleSpanProcessor.create(exporter)) + .build()) + .build(); + StorageOptions storageOptions = + storage.getOptions().toBuilder().setOpenTelemetry(openTelemetrySdk).build(); + + try (Storage storage = storageOptions.getService()) { + try { + storage.get("test"); + } catch (StorageException e) { + // Expected 403 Forbidden + } + Thread.sleep(800); + try { + storage.get("test"); + } catch (StorageException e) { + // Expected 403 Forbidden + } + } + + assertThat(exporter.getExportedSpans().size()).isAtLeast(2); + SpanData getSpan1 = exporter.getExportedSpans().get(0); + SpanData getSpan2 = exporter.getExportedSpans().get(1); + + assertAll( + () -> + assertThat(getAttributeValue(getSpan2, "gcp.resource.destination.id")) + .isEqualTo("projects/_/buckets/test"), + () -> + assertThat(getAttributeValue(getSpan2, "gcp.resource.destination.location")) + .isEqualTo("global")); + } + @Test public void noOpDoesNothing() { assertThat(storage.getOptions().getOpenTelemetry()).isSameInstanceAs(OpenTelemetry.noop()); diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java index fe04abd2c560..fb3e6eee402c 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java @@ -17,23 +17,28 @@ package com.google.cloud.storage; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.mockito.Mockito.mock; import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.api.trace.Tracer; +import java.math.BigInteger; +import java.util.concurrent.TimeUnit; +import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; public class OtelStorageDecoratorAcoUnitTest { - @Test - public void testCacheAndProxyDecorationFlow() throws Exception { - Storage mockStorage = mock(Storage.class); + private OpenTelemetry mockOtel; - OpenTelemetry mockOtel = mock(OpenTelemetry.class); + @Before + public void setUp() { + mockOtel = mock(OpenTelemetry.class); Tracer mockTracer = mock(Tracer.class); SpanBuilder mockSpanBuilder = mock(SpanBuilder.class); Span mockSpan = mock(Span.class); @@ -43,20 +48,173 @@ public void testCacheAndProxyDecorationFlow() throws Exception { Mockito.when(mockTracer.spanBuilder(Mockito.anyString())).thenReturn(mockSpanBuilder); Mockito.when(mockSpanBuilder.setAllAttributes(Mockito.any())).thenReturn(mockSpanBuilder); Mockito.when(mockSpanBuilder.startSpan()).thenReturn(mockSpan); + } + + @Test + public void testAcoSuccessFlow() throws Exception { + Storage mockStorage = mock(Storage.class); + Bucket mockBucket = mock(Bucket.class); + + Mockito.when(mockStorage.get("success-bucket")).thenReturn(mockBucket); + Mockito.when(mockBucket.getProject()).thenReturn(BigInteger.valueOf(12345)); + Mockito.when(mockBucket.getLocation()).thenReturn("us-east1"); + Mockito.when(mockBucket.getLocationType()).thenReturn("region"); + + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "success-bucket"); + + // Wait for background task to finish cleanly + osd.cacheExecutor.shutdown(); + osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("success-bucket"); + assertNotNull(meta); + assertEquals("projects/12345/buckets/success-bucket", meta.resource); + assertEquals("us-east1", meta.location); + assertFalse(meta.fetchPending); + } + + @Test + public void testAco404NotFoundFlowWithException() throws Exception { + Storage mockStorage = mock(Storage.class); + StorageException ex = new StorageException(404, "Bucket not found"); + Mockito.when(mockStorage.get("nonexistent-bucket")).thenThrow(ex); + + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "nonexistent-bucket"); + + // Wait for background task to finish + osd.cacheExecutor.shutdown(); + osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + + // Verified not found -> Entry must be cleanly evicted (null) + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); + assertNull(meta); + } + + @Test + public void testAco404NotFoundFlowWithNull() throws Exception { + Storage mockStorage = mock(Storage.class); + Mockito.when(mockStorage.get("nonexistent-bucket")).thenReturn(null); + + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "nonexistent-bucket"); + + // Wait for background task to finish + osd.cacheExecutor.shutdown(); + osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + + // Verified not found -> Entry must be cleanly evicted (null) + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); + assertNull(meta); + } - Storage decoratedStorage = OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - assertNotNull(decoratedStorage); + @Test + public void testAco403ForbiddenFlow() throws Exception { + Storage mockStorage = mock(Storage.class); + StorageException ex = new StorageException(403, "Access Denied"); + Mockito.when(mockStorage.get("forbidden-bucket")).thenThrow(ex); + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - AcoSpanBuilder.checkCacheAndTriggerFetch(osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "test-poc-bucket"); - BucketMetadataCache cache = osd.bucketMetadataCache; - BucketMetadataCache.BucketMetadata meta = cache.get("test-poc-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "forbidden-bucket"); + + // Wait for background task to finish + osd.cacheExecutor.shutdown(); + osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + + // Forbidden -> Fallback values retained with pending = false (Do Not Retry) + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("forbidden-bucket"); assertNotNull(meta); - assertEquals("projects/_/buckets/test-poc-bucket", meta.resource); + assertEquals("projects/_/buckets/forbidden-bucket", meta.resource); assertEquals("global", meta.location); + assertFalse(meta.fetchPending); + } + + @Test + public void testAcoThunderingHerdProtection() throws Exception { + Storage mockStorage = mock(Storage.class); + Mockito.when(mockStorage.get("concurrent-bucket")) + .thenAnswer( + invocation -> { + Thread.sleep(100); + return null; + }); + + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + + // Trigger twice concurrently + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "concurrent-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "concurrent-bucket"); + + // Wait for background tasks + osd.cacheExecutor.shutdown(); + osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + + // Verify get was called exactly once (no duplicate fetches) + Mockito.verify(mockStorage, Mockito.times(1)).get("concurrent-bucket"); + } + + @Test + public void testAcoAcoSpanEndSkipsPending() throws Exception { + Storage mockStorage = mock(Storage.class); + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + + // Manually put a pending placeholder entry + osd.bucketMetadataCache.put( + "pending-bucket", "projects/_/buckets/pending-bucket", "global", true); + + Span mockSpan = mock(Span.class); + AcoSpan acoSpan = new AcoSpan(mockSpan, "pending-bucket", osd); + + // Call end() while pending + acoSpan.end(); + + // Verify OTel span setAttribute was never called since cache entry was pending + Mockito.verify(mockSpan, Mockito.never()) + .setAttribute(Mockito.anyString(), Mockito.anyString()); + } + + @Test + public void testAcoAcoSpanEndAppliesResolved() throws Exception { + Storage mockStorage = mock(Storage.class); + Storage decoratedStorage = + OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + + // Manually put a resolved non-pending entry + osd.bucketMetadataCache.put( + "resolved-bucket", "projects/123/buckets/resolved-bucket", "us-east1", false); + + Span mockSpan = mock(Span.class); + AcoSpan acoSpan = new AcoSpan(mockSpan, "resolved-bucket", osd); + + acoSpan.end(); - cache.remove("test-poc-bucket"); - org.junit.Assert.assertNull(cache.get("test-poc-bucket")); + // Verify OTel span attributes were set successfully + Mockito.verify(mockSpan) + .setAttribute("gcp.resource.destination.id", "projects/123/buckets/resolved-bucket"); + Mockito.verify(mockSpan).setAttribute("gcp.resource.destination.location", "us-east1"); } } From 64a8fca970eba51a2498aa64ea1d5be543f079b8 Mon Sep 17 00:00:00 2001 From: nidhiii-27 Date: Wed, 20 May 2026 12:37:56 +0530 Subject: [PATCH 08/10] modify MPUClientDecorator and executor service --- .../google/cloud/storage/AcoSpanBuilder.java | 27 ++++++++++++------- .../cloud/storage/MultipartUploadClient.java | 5 +++- .../OtelMultipartUploadClientDecorator.java | 18 ++++++++++--- .../cloud/storage/OtelStorageDecorator.java | 3 ++- .../cloud/storage/ITOpenTelemetryMPUTest.java | 23 ++++++++++++++++ .../OtelStorageDecoratorAcoUnitTest.java | 27 +++++++++++-------- 6 files changed, 77 insertions(+), 26 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java index 1270a61368af..646ffcfb469d 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -26,7 +26,6 @@ import io.opentelemetry.context.Context; import java.util.Locale; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; @@ -133,14 +132,24 @@ public SpanBuilder addLink(SpanContext c, Attributes a) { } static ExecutorService newCacheExecutor() { - return Executors.newFixedThreadPool( - 4, - r -> { - Thread t = new Thread(r); - t.setDaemon(true); - t.setName("gcs-aco-metadata-cache-pool"); - return t; - }); + int poolSize = Math.max(4, Runtime.getRuntime().availableProcessors()); + java.util.concurrent.ThreadPoolExecutor executor = + new java.util.concurrent.ThreadPoolExecutor( + poolSize, // core pool size dynamically scaled based on CPU cores + poolSize, // max pool size dynamically scaled based on CPU cores + 5L, // 5 seconds keep-alive: terminates threads quickly when done + TimeUnit.SECONDS, + new java.util.concurrent + .LinkedBlockingQueue<>(), // Unbounded queue ensures no tasks are ever rejected or + // lost + r -> { + Thread t = new Thread(r); + t.setDaemon(true); + t.setName("gcs-aco-metadata-cache-pool"); + return t; + }); + executor.allowCoreThreadTimeOut(true); + return executor; } static String extractBucketName(String uri) { diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClient.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClient.java index cf8353f1f6f5..7edc5acc0abb 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClient.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/MultipartUploadClient.java @@ -118,7 +118,10 @@ public static MultipartUploadClient create(MultipartUploadSettings config) { options.createRetrier(), MultipartUploadHttpRequestManager.createFrom(options), options.getRetryAlgorithmManager()); + Storage service = options.getService(); + OtelStorageDecorator osd = + service instanceof OtelStorageDecorator ? (OtelStorageDecorator) service : null; return OtelMultipartUploadClientDecorator.decorate( - client, options.getOpenTelemetry(), Transport.HTTP); + client, osd, options.getOpenTelemetry(), Transport.HTTP); } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java index a3832b2e9529..34f15a5dd8ac 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelMultipartUploadClientDecorator.java @@ -49,11 +49,18 @@ final class OtelMultipartUploadClientDecorator extends MultipartUploadClient { private final Tracer tracer; private OtelMultipartUploadClientDecorator( - MultipartUploadClient delegate, OpenTelemetry otel, Attributes baseAttributes) { + MultipartUploadClient delegate, + OtelStorageDecorator parentDecorator, + OpenTelemetry otel, + Attributes baseAttributes) { this.delegate = delegate; this.tracer = OtelStorageDecorator.TracerDecorator.decorate( - null, null, otel, baseAttributes, MultipartUploadClient.class.getName() + "/"); + parentDecorator, + null, + otel, + baseAttributes, + MultipartUploadClient.class.getName() + "/"); } @Override @@ -172,7 +179,10 @@ public ListMultipartUploadsResponse listMultipartUploads(ListMultipartUploadsReq } static MultipartUploadClient decorate( - MultipartUploadClient delegate, OpenTelemetry otel, Transport transport) { + MultipartUploadClient delegate, + OtelStorageDecorator parentDecorator, + OpenTelemetry otel, + Transport transport) { if (otel == OpenTelemetry.noop()) { return delegate; } @@ -185,6 +195,6 @@ static MultipartUploadClient decorate( .put("rpc.system", "XML") .put("service.name", "storage.googleapis.com") .build(); - return new OtelMultipartUploadClientDecorator(delegate, otel, baseAttributes); + return new OtelMultipartUploadClientDecorator(delegate, parentDecorator, otel, baseAttributes); } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index eb0edcb52705..5b703fdae91e 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -1430,7 +1430,8 @@ public boolean deleteNotification(String bucket, String notificationId) { public void close() throws Exception { try { bucketMetadataCache.clear(); - cacheExecutor.shutdown(); + cacheExecutor.shutdownNow(); + cacheExecutor.awaitTermination(5, TimeUnit.MINUTES); } finally { delegate.close(); } diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryMPUTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryMPUTest.java index e1a83ba6ebda..69a986ac639c 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryMPUTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryMPUTest.java @@ -132,17 +132,40 @@ public void checkMPUInstrumentation() throws Exception { assertThat(uploadSpan.getAttributes().get(AttributeKey.stringKey("gsutil.uri"))) .isEqualTo(String.format("gs://%s/%s", bucket.getName(), objectName)); assertThat(uploadSpan.getAttributes().get(AttributeKey.longKey("partNumber"))).isEqualTo(1); + assertThat( + uploadSpan.getAttributes().get(AttributeKey.stringKey("gcp.resource.destination.id"))) + .contains("buckets/" + bucket.getName()); + assertThat( + uploadSpan + .getAttributes() + .get(AttributeKey.stringKey("gcp.resource.destination.location"))) + .isNotEqualTo("global"); SpanData completeSpan = spans.get(2); assertThat(completeSpan.getName()) .isEqualTo("com.google.cloud.storage.MultipartUploadClient/completeMultipartUpload"); assertThat(completeSpan.getAttributes().get(AttributeKey.stringKey("gsutil.uri"))) .isEqualTo(String.format("gs://%s/%s", bucket.getName(), objectName)); + assertThat( + completeSpan.getAttributes().get(AttributeKey.stringKey("gcp.resource.destination.id"))) + .contains("buckets/" + bucket.getName()); + assertThat( + completeSpan + .getAttributes() + .get(AttributeKey.stringKey("gcp.resource.destination.location"))) + .isNotEqualTo("global"); SpanData listSpan = spans.get(3); assertThat(listSpan.getName()) .isEqualTo("com.google.cloud.storage.MultipartUploadClient/listMultipartUploads"); assertThat(listSpan.getAttributes().get(AttributeKey.stringKey("gsutil.uri"))) .isEqualTo(String.format("gs://%s/", bucket.getName())); + assertThat(listSpan.getAttributes().get(AttributeKey.stringKey("gcp.resource.destination.id"))) + .contains("buckets/" + bucket.getName()); + assertThat( + listSpan + .getAttributes() + .get(AttributeKey.stringKey("gcp.resource.destination.location"))) + .isNotEqualTo("global"); } } diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java index fb3e6eee402c..25f8528b983a 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java @@ -27,7 +27,6 @@ import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.api.trace.Tracer; import java.math.BigInteger; -import java.util.concurrent.TimeUnit; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; @@ -68,8 +67,7 @@ public void testAcoSuccessFlow() throws Exception { osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "success-bucket"); // Wait for background task to finish cleanly - osd.cacheExecutor.shutdown(); - osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + waitForCache(osd, "success-bucket"); BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("success-bucket"); assertNotNull(meta); @@ -92,8 +90,7 @@ public void testAco404NotFoundFlowWithException() throws Exception { osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "nonexistent-bucket"); // Wait for background task to finish - osd.cacheExecutor.shutdown(); - osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + waitForCache(osd, "nonexistent-bucket"); // Verified not found -> Entry must be cleanly evicted (null) BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); @@ -113,8 +110,7 @@ public void testAco404NotFoundFlowWithNull() throws Exception { osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "nonexistent-bucket"); // Wait for background task to finish - osd.cacheExecutor.shutdown(); - osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + waitForCache(osd, "nonexistent-bucket"); // Verified not found -> Entry must be cleanly evicted (null) BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); @@ -135,8 +131,7 @@ public void testAco403ForbiddenFlow() throws Exception { osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "forbidden-bucket"); // Wait for background task to finish - osd.cacheExecutor.shutdown(); - osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + waitForCache(osd, "forbidden-bucket"); // Forbidden -> Fallback values retained with pending = false (Do Not Retry) BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("forbidden-bucket"); @@ -167,8 +162,7 @@ public void testAcoThunderingHerdProtection() throws Exception { osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "concurrent-bucket"); // Wait for background tasks - osd.cacheExecutor.shutdown(); - osd.cacheExecutor.awaitTermination(5, TimeUnit.SECONDS); + waitForCache(osd, "concurrent-bucket"); // Verify get was called exactly once (no duplicate fetches) Mockito.verify(mockStorage, Mockito.times(1)).get("concurrent-bucket"); @@ -217,4 +211,15 @@ public void testAcoAcoSpanEndAppliesResolved() throws Exception { .setAttribute("gcp.resource.destination.id", "projects/123/buckets/resolved-bucket"); Mockito.verify(mockSpan).setAttribute("gcp.resource.destination.location", "us-east1"); } + + private void waitForCache(OtelStorageDecorator osd, String bucketName) throws Exception { + for (int i = 0; i < 100; i++) { + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get(bucketName); + if (meta == null || !meta.fetchPending) { + return; + } + Thread.sleep(50); + } + throw new AssertionError("Timeout waiting for cache background fetch for: " + bucketName); + } } From bdde0aa3c0dfd20e3d7ca3be5f952989c642d89e Mon Sep 17 00:00:00 2001 From: nidhiii-27 Date: Wed, 20 May 2026 14:04:57 +0530 Subject: [PATCH 09/10] modify unit test and address review comments --- .../com/google/cloud/storage/AcoSpan.java | 16 +- .../google/cloud/storage/AcoSpanBuilder.java | 14 +- .../cloud/storage/BucketMetadataCache.java | 40 +++- .../cloud/storage/OtelStorageDecorator.java | 26 ++- .../OtelStorageDecoratorAcoUnitTest.java | 195 +++++++++--------- 5 files changed, 168 insertions(+), 123 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java index d2972fab8900..4c89a0ca6e09 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpan.java @@ -59,21 +59,18 @@ public void end(long timestamp, TimeUnit unit) { @Override public Span recordException(Throwable exception) { delegate.recordException(exception); - if (exception instanceof StorageException && parent != null) { - StorageException se = (StorageException) exception; - if (se.getCode() == 404 && se.getMessage() != null) { - String msg = se.getMessage().toLowerCase(java.util.Locale.US); - if (msg.contains("bucket not found") || msg.contains("bucket does not exist")) { - parent.bucketMetadataCache.remove(bucketName); - } - } - } + handleException(exception); return this; } @Override public Span recordException(Throwable exception, Attributes attributes) { delegate.recordException(exception, attributes); + handleException(exception); + return this; + } + + private void handleException(Throwable exception) { if (exception instanceof StorageException && parent != null) { StorageException se = (StorageException) exception; if (se.getCode() == 404 && se.getMessage() != null) { @@ -83,7 +80,6 @@ public Span recordException(Throwable exception, Attributes attributes) { } } } - return this; } @Override diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java index 646ffcfb469d..eccd1427c8f3 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -33,6 +33,8 @@ final class AcoSpanBuilder implements SpanBuilder { private static final Logger LOGGER = Logger.getLogger(AcoSpanBuilder.class.getName()); + private static final String MULTI_REGION = "multi-region"; + private static final String DUAL_REGION = "dual-region"; private final SpanBuilder delegate; private final OtelStorageDecorator parent; @@ -71,7 +73,7 @@ public SpanBuilder setAttribute(AttributeKey key, T value) { public Span startSpan() { if (bucketName != null && parent != null) { checkCacheAndTriggerFetch( - parent.delegate, parent.bucketMetadataCache, parent.cacheExecutor, bucketName); + parent.delegate, parent.bucketMetadataCache, parent.getCacheExecutor(), bucketName); return new AcoSpan(delegate.startSpan(), bucketName, parent); } return delegate.startSpan(); @@ -139,15 +141,15 @@ static ExecutorService newCacheExecutor() { poolSize, // max pool size dynamically scaled based on CPU cores 5L, // 5 seconds keep-alive: terminates threads quickly when done TimeUnit.SECONDS, - new java.util.concurrent - .LinkedBlockingQueue<>(), // Unbounded queue ensures no tasks are ever rejected or - // lost + new java.util.concurrent.LinkedBlockingQueue<>(10000), // Bounded queue to prevent OOM r -> { Thread t = new Thread(r); t.setDaemon(true); t.setName("gcs-aco-metadata-cache-pool"); return t; - }); + }, + new java.util.concurrent.ThreadPoolExecutor + .DiscardPolicy()); // Best-effort: silently discard on overflow executor.allowCoreThreadTimeOut(true); return executor; } @@ -185,7 +187,7 @@ static Tuple fetch(Storage delegate, String bucketName) { ? bucket.getLocationType().toLowerCase(Locale.US) : "region"; - if ("multi-region".equals(locationType) || "dual-region".equals(locationType)) { + if (MULTI_REGION.equals(locationType) || DUAL_REGION.equals(locationType)) { location = "global"; } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java index c9d48dcfe374..c56a930614d0 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketMetadataCache.java @@ -19,11 +19,12 @@ import com.google.cloud.Tuple; import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.locks.ReentrantLock; final class BucketMetadataCache { private static final int DEFAULT_CAPACITY = 10000; - private final Object lock = new Object(); + private final ReentrantLock lock = new ReentrantLock(); private final Map cache; BucketMetadataCache(int capacity) { @@ -36,49 +37,70 @@ protected boolean removeEldestEntry(Map.Entry eldest) { }; } - static BucketMetadataCache getbucketmetadatacache() { + static BucketMetadataCache getBucketMetadataCache() { return new BucketMetadataCache(DEFAULT_CAPACITY); } BucketMetadata get(String bucketName) { - synchronized (lock) { + lock.lock(); + try { return cache.get(bucketName); + } finally { + lock.unlock(); } } void put(String bucketName, BucketMetadata metadata) { - synchronized (lock) { + lock.lock(); + try { cache.put(bucketName, metadata); + } finally { + lock.unlock(); } } void put(String bucketName, String resource, String location, boolean pending) { - synchronized (lock) { + lock.lock(); + try { cache.put(bucketName, new BucketMetadata(resource, location, pending)); + } finally { + lock.unlock(); } } void put(String bucketName, Tuple layout, boolean pending) { - synchronized (lock) { + lock.lock(); + try { cache.put(bucketName, new BucketMetadata(layout.x(), layout.y(), pending)); + } finally { + lock.unlock(); } } void remove(String bucketName) { - synchronized (lock) { + lock.lock(); + try { cache.remove(bucketName); + } finally { + lock.unlock(); } } void clear() { - synchronized (lock) { + lock.lock(); + try { cache.clear(); + } finally { + lock.unlock(); } } boolean containsKey(String bucketName) { - synchronized (lock) { + lock.lock(); + try { return cache.containsKey(bucketName); + } finally { + lock.unlock(); } } diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java index 5b703fdae91e..77f3aa7e4bec 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/OtelStorageDecorator.java @@ -77,7 +77,7 @@ final class OtelStorageDecorator implements Storage { private final Attributes baseAttributes; private final Tracer tracer; final BucketMetadataCache bucketMetadataCache; - final ExecutorService cacheExecutor; + private volatile ExecutorService cacheExecutor; private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes baseAttributes) { this.delegate = delegate; @@ -85,8 +85,20 @@ private OtelStorageDecorator(Storage delegate, OpenTelemetry otel, Attributes ba this.baseAttributes = baseAttributes; this.tracer = TracerDecorator.decorate(this, null, otel, baseAttributes, Storage.class.getName() + "/"); - this.bucketMetadataCache = BucketMetadataCache.getbucketmetadatacache(); - this.cacheExecutor = AcoSpanBuilder.newCacheExecutor(); + this.bucketMetadataCache = BucketMetadataCache.getBucketMetadataCache(); + } + + ExecutorService getCacheExecutor() { + ExecutorService result = cacheExecutor; + if (result == null) { + synchronized (this) { + result = cacheExecutor; + if (result == null) { + cacheExecutor = result = AcoSpanBuilder.newCacheExecutor(); + } + } + } + return result; } @Override @@ -1430,8 +1442,12 @@ public boolean deleteNotification(String bucket, String notificationId) { public void close() throws Exception { try { bucketMetadataCache.clear(); - cacheExecutor.shutdownNow(); - cacheExecutor.awaitTermination(5, TimeUnit.MINUTES); + synchronized (this) { + if (cacheExecutor != null) { + cacheExecutor.shutdownNow(); + cacheExecutor.awaitTermination(5, TimeUnit.MINUTES); + } + } } finally { delegate.close(); } diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java index 25f8528b983a..77895f04f4cb 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/OtelStorageDecoratorAcoUnitTest.java @@ -27,6 +27,7 @@ import io.opentelemetry.api.trace.SpanBuilder; import io.opentelemetry.api.trace.Tracer; import java.math.BigInteger; +import java.util.concurrent.TimeUnit; import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; @@ -59,21 +60,24 @@ public void testAcoSuccessFlow() throws Exception { Mockito.when(mockBucket.getLocation()).thenReturn("us-east1"); Mockito.when(mockBucket.getLocationType()).thenReturn("region"); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - AcoSpanBuilder.checkCacheAndTriggerFetch( - osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "success-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.getCacheExecutor(), "success-bucket"); - // Wait for background task to finish cleanly - waitForCache(osd, "success-bucket"); + // Wait for background task to finish cleanly + osd.getCacheExecutor().shutdown(); + osd.getCacheExecutor().awaitTermination(5, TimeUnit.SECONDS); - BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("success-bucket"); - assertNotNull(meta); - assertEquals("projects/12345/buckets/success-bucket", meta.resource); - assertEquals("us-east1", meta.location); - assertFalse(meta.fetchPending); + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("success-bucket"); + assertNotNull(meta); + assertEquals("projects/12345/buckets/success-bucket", meta.resource); + assertEquals("us-east1", meta.location); + assertFalse(meta.fetchPending); + } } @Test @@ -82,19 +86,22 @@ public void testAco404NotFoundFlowWithException() throws Exception { StorageException ex = new StorageException(404, "Bucket not found"); Mockito.when(mockStorage.get("nonexistent-bucket")).thenThrow(ex); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - AcoSpanBuilder.checkCacheAndTriggerFetch( - osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "nonexistent-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.getCacheExecutor(), "nonexistent-bucket"); - // Wait for background task to finish - waitForCache(osd, "nonexistent-bucket"); + // Wait for background task to finish + osd.getCacheExecutor().shutdown(); + osd.getCacheExecutor().awaitTermination(5, TimeUnit.SECONDS); - // Verified not found -> Entry must be cleanly evicted (null) - BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); - assertNull(meta); + // Verified not found -> Entry must be cleanly evicted (null) + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); + assertNull(meta); + } } @Test @@ -102,19 +109,22 @@ public void testAco404NotFoundFlowWithNull() throws Exception { Storage mockStorage = mock(Storage.class); Mockito.when(mockStorage.get("nonexistent-bucket")).thenReturn(null); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - AcoSpanBuilder.checkCacheAndTriggerFetch( - osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "nonexistent-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.getCacheExecutor(), "nonexistent-bucket"); - // Wait for background task to finish - waitForCache(osd, "nonexistent-bucket"); + // Wait for background task to finish + osd.getCacheExecutor().shutdown(); + osd.getCacheExecutor().awaitTermination(5, TimeUnit.SECONDS); - // Verified not found -> Entry must be cleanly evicted (null) - BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); - assertNull(meta); + // Verified not found -> Entry must be cleanly evicted (null) + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("nonexistent-bucket"); + assertNull(meta); + } } @Test @@ -123,22 +133,25 @@ public void testAco403ForbiddenFlow() throws Exception { StorageException ex = new StorageException(403, "Access Denied"); Mockito.when(mockStorage.get("forbidden-bucket")).thenThrow(ex); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - AcoSpanBuilder.checkCacheAndTriggerFetch( - osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "forbidden-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.getCacheExecutor(), "forbidden-bucket"); - // Wait for background task to finish - waitForCache(osd, "forbidden-bucket"); + // Wait for background task to finish + osd.getCacheExecutor().shutdown(); + osd.getCacheExecutor().awaitTermination(5, TimeUnit.SECONDS); - // Forbidden -> Fallback values retained with pending = false (Do Not Retry) - BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("forbidden-bucket"); - assertNotNull(meta); - assertEquals("projects/_/buckets/forbidden-bucket", meta.resource); - assertEquals("global", meta.location); - assertFalse(meta.fetchPending); + // Forbidden -> Fallback values retained with pending = false (Do Not Retry) + BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get("forbidden-bucket"); + assertNotNull(meta); + assertEquals("projects/_/buckets/forbidden-bucket", meta.resource); + assertEquals("global", meta.location); + assertFalse(meta.fetchPending); + } } @Test @@ -151,75 +164,71 @@ public void testAcoThunderingHerdProtection() throws Exception { return null; }); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - // Trigger twice concurrently - AcoSpanBuilder.checkCacheAndTriggerFetch( - osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "concurrent-bucket"); - AcoSpanBuilder.checkCacheAndTriggerFetch( - osd.delegate, osd.bucketMetadataCache, osd.cacheExecutor, "concurrent-bucket"); + // Trigger twice concurrently + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.getCacheExecutor(), "concurrent-bucket"); + AcoSpanBuilder.checkCacheAndTriggerFetch( + osd.delegate, osd.bucketMetadataCache, osd.getCacheExecutor(), "concurrent-bucket"); - // Wait for background tasks - waitForCache(osd, "concurrent-bucket"); + // Wait for background tasks + osd.getCacheExecutor().shutdown(); + osd.getCacheExecutor().awaitTermination(5, TimeUnit.SECONDS); - // Verify get was called exactly once (no duplicate fetches) - Mockito.verify(mockStorage, Mockito.times(1)).get("concurrent-bucket"); + // Verify get was called exactly once (no duplicate fetches) + Mockito.verify(mockStorage, Mockito.times(1)).get("concurrent-bucket"); + } } @Test public void testAcoAcoSpanEndSkipsPending() throws Exception { Storage mockStorage = mock(Storage.class); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - // Manually put a pending placeholder entry - osd.bucketMetadataCache.put( - "pending-bucket", "projects/_/buckets/pending-bucket", "global", true); + // Manually put a pending placeholder entry + osd.bucketMetadataCache.put( + "pending-bucket", "projects/_/buckets/pending-bucket", "global", true); - Span mockSpan = mock(Span.class); - AcoSpan acoSpan = new AcoSpan(mockSpan, "pending-bucket", osd); + Span mockSpan = mock(Span.class); + AcoSpan acoSpan = new AcoSpan(mockSpan, "pending-bucket", osd); - // Call end() while pending - acoSpan.end(); + // Call end() while pending + acoSpan.end(); - // Verify OTel span setAttribute was never called since cache entry was pending - Mockito.verify(mockSpan, Mockito.never()) - .setAttribute(Mockito.anyString(), Mockito.anyString()); + // Verify OTel span setAttribute was never called since cache entry was pending + Mockito.verify(mockSpan, Mockito.never()) + .setAttribute(Mockito.anyString(), Mockito.anyString()); + } } @Test public void testAcoAcoSpanEndAppliesResolved() throws Exception { Storage mockStorage = mock(Storage.class); - Storage decoratedStorage = - OtelStorageDecorator.decorate(mockStorage, mockOtel, TransportCompatibility.Transport.HTTP); - OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; + try (Storage decoratedStorage = + OtelStorageDecorator.decorate( + mockStorage, mockOtel, TransportCompatibility.Transport.HTTP)) { + OtelStorageDecorator osd = (OtelStorageDecorator) decoratedStorage; - // Manually put a resolved non-pending entry - osd.bucketMetadataCache.put( - "resolved-bucket", "projects/123/buckets/resolved-bucket", "us-east1", false); + // Manually put a resolved non-pending entry + osd.bucketMetadataCache.put( + "resolved-bucket", "projects/123/buckets/resolved-bucket", "us-east1", false); - Span mockSpan = mock(Span.class); - AcoSpan acoSpan = new AcoSpan(mockSpan, "resolved-bucket", osd); + Span mockSpan = mock(Span.class); + AcoSpan acoSpan = new AcoSpan(mockSpan, "resolved-bucket", osd); - acoSpan.end(); - - // Verify OTel span attributes were set successfully - Mockito.verify(mockSpan) - .setAttribute("gcp.resource.destination.id", "projects/123/buckets/resolved-bucket"); - Mockito.verify(mockSpan).setAttribute("gcp.resource.destination.location", "us-east1"); - } + acoSpan.end(); - private void waitForCache(OtelStorageDecorator osd, String bucketName) throws Exception { - for (int i = 0; i < 100; i++) { - BucketMetadataCache.BucketMetadata meta = osd.bucketMetadataCache.get(bucketName); - if (meta == null || !meta.fetchPending) { - return; - } - Thread.sleep(50); + // Verify OTel span attributes were set successfully + Mockito.verify(mockSpan) + .setAttribute("gcp.resource.destination.id", "projects/123/buckets/resolved-bucket"); + Mockito.verify(mockSpan).setAttribute("gcp.resource.destination.location", "us-east1"); } - throw new AssertionError("Timeout waiting for cache background fetch for: " + bucketName); } } From ede40e315dcdb990294fd6be8a9e188ac2f68a0d Mon Sep 17 00:00:00 2001 From: nidhiii-27 Date: Wed, 20 May 2026 16:23:33 +0530 Subject: [PATCH 10/10] handle transient failures in metadata fetching --- .../main/java/com/google/cloud/storage/AcoSpanBuilder.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java index eccd1427c8f3..790c494643a5 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/AcoSpanBuilder.java @@ -226,9 +226,13 @@ static void checkCacheAndTriggerFetch( bucketName, "projects/_/buckets/" + bucketName, "global", false); } else { LOGGER.log(Level.WARNING, "Background GetBucket failed", e); + // Transient failure -> Evict cache entry to allow future retries + bucketMetadataCache.remove(bucketName); } } catch (Exception e) { LOGGER.log(Level.WARNING, "Background GetBucket failed", e); + // Transient failure -> Evict cache entry to allow future retries + bucketMetadataCache.remove(bucketName); } }); }