From 0fb2f1823f9eff8534f15240321003f120fed3f4 Mon Sep 17 00:00:00 2001 From: JesseLovelace <43148100+JesseLovelace@users.noreply.github.com> Date: Thu, 27 Oct 2022 14:51:19 -0700 Subject: [PATCH] feat: update retries for Notifications (#1734) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: update retries for Notifications * review comments * format * deps * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot --- README.md | 4 +- google-cloud-storage/pom.xml | 7 ++ .../storage/HttpRetryAlgorithmManager.java | 17 +++ .../com/google/cloud/storage/StorageImpl.java | 108 +++++------------- .../conformance/retry/CtxFunctions.java | 36 ++++++ .../conformance/retry/RpcMethodMappings.java | 82 ++++++++++++- .../storage/conformance/retry/State.java | 42 +++++++ .../retry/TestRetryConformance.java | 9 ++ renovate.json | 3 +- 9 files changed, 221 insertions(+), 87 deletions(-) diff --git a/README.md b/README.md index 4f61c4f7c..3fd96c2c7 100644 --- a/README.md +++ b/README.md @@ -56,13 +56,13 @@ implementation 'com.google.cloud:google-cloud-storage' If you are using Gradle without BOM, add this to your dependencies: ```Groovy -implementation 'com.google.cloud:google-cloud-storage:2.13.1' +implementation 'com.google.cloud:google-cloud-storage:2.14.0' ``` If you are using SBT, add this to your dependencies: ```Scala -libraryDependencies += "com.google.cloud" % "google-cloud-storage" % "2.13.1" +libraryDependencies += "com.google.cloud" % "google-cloud-storage" % "2.14.0" ``` ## Authentication diff --git a/google-cloud-storage/pom.xml b/google-cloud-storage/pom.xml index 108312422..bff7c7be3 100644 --- a/google-cloud-storage/pom.xml +++ b/google-cloud-storage/pom.xml @@ -16,6 +16,7 @@ google-cloud-storage + 1.102.22 0.99.0 5.9.1 @@ -185,6 +186,12 @@ com.google.api.grpc grpc-google-iam-v1 + + com.google.api.grpc + proto-google-cloud-pubsub-v1 + test + ${pubsub-proto.version} + com.google.cloud google-cloud-core diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpRetryAlgorithmManager.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpRetryAlgorithmManager.java index 90ff2ee30..5a7df8621 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpRetryAlgorithmManager.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpRetryAlgorithmManager.java @@ -20,6 +20,7 @@ import com.google.api.services.storage.model.Bucket; import com.google.api.services.storage.model.BucketAccessControl; import com.google.api.services.storage.model.HmacKeyMetadata; +import com.google.api.services.storage.model.Notification; import com.google.api.services.storage.model.ObjectAccessControl; import com.google.api.services.storage.model.Policy; import com.google.api.services.storage.model.StorageObject; @@ -243,4 +244,20 @@ public ResultRetryAlgorithm getForResumableUploadSessionWrite( public ResultRetryAlgorithm getForServiceAccountGet(String pb) { return retryStrategy.getIdempotentHandler(); } + + public ResultRetryAlgorithm getForNotificationCreate(String bucket, Notification pb) { + return retryStrategy.getNonidempotentHandler(); + } + + public ResultRetryAlgorithm getForNotificationGet(String bucket, String notificationId) { + return retryStrategy.getIdempotentHandler(); + } + + public ResultRetryAlgorithm getForNotificationList(String bucket) { + return retryStrategy.getIdempotentHandler(); + } + + public ResultRetryAlgorithm getForNotificationDelete(String bucket, String notificationId) { + return retryStrategy.getIdempotentHandler(); + } } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index a353869ad..7241d76cb 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -16,7 +16,6 @@ package com.google.cloud.storage; -import static com.google.cloud.RetryHelper.runWithRetries; import static com.google.cloud.storage.SignedUrlEncodingHelper.Rfc3986UriEncode; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; @@ -36,7 +35,6 @@ import com.google.cloud.PageImpl.NextPageFetcher; import com.google.cloud.Policy; import com.google.cloud.ReadChannel; -import com.google.cloud.RetryHelper.RetryHelperException; import com.google.cloud.WriteChannel; import com.google.cloud.storage.Acl.Entity; import com.google.cloud.storage.HmacKey.HmacKeyMetadata; @@ -1483,96 +1481,46 @@ public Notification createNotification( final String bucket, final NotificationInfo notificationInfo) { final com.google.api.services.storage.model.Notification notificationPb = codecs.notificationInfo().encode(notificationInfo); - try { - return codecs - .notificationInfo() - .decode( - runWithRetries( - new Callable() { - @Override - public com.google.api.services.storage.model.Notification call() { - return storageRpc.createNotification(bucket, notificationPb); - } - }, - getOptions().getRetrySettings(), - EXCEPTION_HANDLER, - getOptions().getClock())) - .asNotification(this); - } catch (RetryHelperException e) { - throw StorageException.translateAndThrow(e); - } + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForNotificationCreate(bucket, notificationPb); + return run( + algorithm, + () -> storageRpc.createNotification(bucket, notificationPb), + n -> codecs.notificationInfo().decode(n).asNotification(this)); } @Override public Notification getNotification(final String bucket, final String notificationId) { - try { - com.google.api.services.storage.model.Notification answer = - runWithRetries( - new Callable() { - @Override - public com.google.api.services.storage.model.Notification call() { - return storageRpc.getNotification(bucket, notificationId); - } - }, - getOptions().getRetrySettings(), - EXCEPTION_HANDLER, - getOptions().getClock()); - return answer == null ? null : codecs.notificationInfo().decode(answer).asNotification(this); - } catch (RetryHelperException e) { - throw StorageException.translateAndThrow(e); - } + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForNotificationGet(bucket, notificationId); + return run( + algorithm, + () -> storageRpc.getNotification(bucket, notificationId), + n -> codecs.notificationInfo().decode(n).asNotification(this)); } @Override public List listNotifications(final String bucket) { - try { - List answer = - runWithRetries( - new Callable>() { - @Override - public List call() { - return storageRpc.listNotifications(bucket); - } - }, - getOptions().getRetrySettings(), - EXCEPTION_HANDLER, - getOptions().getClock()); - return answer == null - ? ImmutableList.of() - : Lists.transform( - answer, - new com.google.common.base.Function< - com.google.api.services.storage.model.Notification, Notification>() { - @Override - public Notification apply( - com.google.api.services.storage.model.Notification notificationPb) { - return codecs - .notificationInfo() - .decode(notificationPb) - .asNotification(getOptions().getService()); - } - }); - } catch (RetryHelperException e) { - throw StorageException.translateAndThrow(e); - } + ResultRetryAlgorithm algorithm = retryAlgorithmManager.getForNotificationList(bucket); + List result = + run( + algorithm, + () -> storageRpc.listNotifications(bucket), + (answer) -> + answer.stream() + .map(n -> codecs.notificationInfo().decode(n).asNotification(this)) + .collect(ImmutableList.toImmutableList())); + return result == null ? ImmutableList.of() : result; } @Override public boolean deleteNotification(final String bucket, final String notificationId) { - try { - return runWithRetries( - new Callable() { - @Override - public Boolean call() { - return storageRpc.deleteNotification(bucket, notificationId); - } - }, - getOptions().getRetrySettings(), - EXCEPTION_HANDLER, - getOptions().getClock()); - } catch (RetryHelperException e) { - throw StorageException.translateAndThrow(e); - } + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForNotificationDelete(bucket, notificationId); + return run( + algorithm, + () -> storageRpc.deleteNotification(bucket, notificationId), + Function.identity()); } @Override diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java index 5e8e04fce..7617be79f 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java @@ -31,13 +31,18 @@ import com.google.cloud.storage.HmacKey; import com.google.cloud.storage.HmacKey.HmacKeyMetadata; import com.google.cloud.storage.HmacKey.HmacKeyState; +import com.google.cloud.storage.NotificationInfo; +import com.google.cloud.storage.NotificationInfo.PayloadFormat; import com.google.cloud.storage.ServiceAccount; import com.google.cloud.storage.Storage; import com.google.cloud.storage.Storage.BlobTargetOption; import com.google.cloud.storage.Storage.ComposeRequest; import com.google.cloud.storage.conformance.retry.Functions.CtxFunction; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableMap; +import com.google.pubsub.v1.TopicName; import java.util.HashSet; +import java.util.Map; /** * Define a set of {@link CtxFunction} which are used in mappings as well as general setup/tear down @@ -173,6 +178,28 @@ static final class ResourceSetup { return s.withHmacKey(hmacKey1).with(hmacKey1.getMetadata()); }); + static final CtxFunction pubsubTopic = + (ctx, c) -> { + String projectId = c.getProjectId(); + TopicName name = TopicName.of(projectId, c.getTopicName()); + return ctx.map(s -> s.with(name)); + }; + + static final CtxFunction notification = + (ctx, c) -> + ctx.map( + state -> { + PayloadFormat format = PayloadFormat.JSON_API_V1; + Map attributes = ImmutableMap.of("label1", "value1"); + NotificationInfo notificationInfo = + NotificationInfo.newBuilder(state.getTopicName().toString()) + .setCustomAttributes(attributes) + .setPayloadFormat(format) + .build(); + return state.with( + ctx.getStorage().createNotification(c.getBucketName(), notificationInfo)); + }); + private static final CtxFunction processResources = (ctx, c) -> { HashSet resources = newHashSet(c.getMethod().getResourcesList()); @@ -192,6 +219,11 @@ static final class ResourceSetup { resources.remove(Resource.HMAC_KEY); } + if (resources.contains(Resource.NOTIFICATION)) { + f = f.andThen(pubsubTopic).andThen(notification); + resources.remove(Resource.NOTIFICATION); + } + if (!resources.isEmpty()) { throw new IllegalStateException( String.format("Unhandled Method Resource [%s]", Joiner.on(", ").join(resources))); @@ -204,6 +236,10 @@ static final class ResourceSetup { (ctx, c) -> ctx.map(s -> s.with(Acl.of(User.ofAllUsers(), Role.READER))); static final CtxFunction defaultSetup = processResources.andThen(allUsersReaderAcl); + + static final CtxFunction pubsubTopicSetup = defaultSetup.andThen(pubsubTopic); + + static final CtxFunction notificationSetup = pubsubTopicSetup.andThen(notification); } static final class ResourceTeardown { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java index 443377d54..458bffddb 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java @@ -17,6 +17,8 @@ package com.google.cloud.storage.conformance.retry; import static com.google.cloud.storage.conformance.retry.CtxFunctions.ResourceSetup.defaultSetup; +import static com.google.cloud.storage.conformance.retry.CtxFunctions.ResourceSetup.notificationSetup; +import static com.google.cloud.storage.conformance.retry.CtxFunctions.ResourceSetup.pubsubTopicSetup; import static com.google.cloud.storage.conformance.retry.CtxFunctions.ResourceSetup.serviceAccount; import static com.google.common.base.Predicates.and; import static com.google.common.base.Predicates.not; @@ -36,6 +38,8 @@ import com.google.cloud.storage.HmacKey.HmacKeyMetadata; import com.google.cloud.storage.HmacKey.HmacKeyState; import com.google.cloud.storage.HttpMethod; +import com.google.cloud.storage.NotificationInfo; +import com.google.cloud.storage.NotificationInfo.PayloadFormat; import com.google.cloud.storage.Storage; import com.google.cloud.storage.Storage.BlobGetOption; import com.google.cloud.storage.Storage.BlobSourceOption; @@ -55,6 +59,7 @@ import com.google.cloud.storage.conformance.retry.RpcMethod.storage.buckets; import com.google.cloud.storage.conformance.retry.RpcMethod.storage.default_object_acl; import com.google.cloud.storage.conformance.retry.RpcMethod.storage.hmacKey; +import com.google.cloud.storage.conformance.retry.RpcMethod.storage.notifications; import com.google.cloud.storage.conformance.retry.RpcMethod.storage.object_acl; import com.google.cloud.storage.conformance.retry.RpcMethod.storage.objects; import com.google.cloud.storage.conformance.retry.RpcMethod.storage.serviceaccount; @@ -89,6 +94,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.List; +import java.util.Map; import java.util.Map.Entry; import java.util.OptionalInt; import java.util.Set; @@ -897,13 +904,80 @@ private static void create(ArrayList a) { static final class Notification { - private static void delete(ArrayList a) {} + private static void delete(ArrayList a) { + a.add( + RpcMethodMapping.newBuilder(248, notifications.delete) + .withSetup(notificationSetup) + .withTest( + (ctx, c) -> + ctx.map( + state -> { + boolean success = + ctx.getStorage() + .deleteNotification( + state.getBucket().getName(), + state.getNotification().getNotificationId()); + assertTrue(success); + return state.with(success); + })) + .build()); + } - private static void get(ArrayList a) {} + private static void get(ArrayList a) { + a.add( + RpcMethodMapping.newBuilder(246, notifications.get) + .withSetup(notificationSetup) + .withTest( + (ctx, c) -> + ctx.map( + state -> { + com.google.cloud.storage.Notification notification = + ctx.getStorage() + .getNotification( + state.getBucket().getName(), + state.getNotification().getNotificationId()); + return state.with(notification); + })) + .build()); + } - private static void insert(ArrayList a) {} + private static void insert(ArrayList a) { + a.add( + RpcMethodMapping.newBuilder(247, notifications.insert) + .withSetup(pubsubTopicSetup) + .withTest( + (ctx, c) -> + ctx.map( + state -> { + PayloadFormat format = PayloadFormat.JSON_API_V1; + Map attributes = ImmutableMap.of("label1", "value1"); + NotificationInfo info = + NotificationInfo.newBuilder(state.getTopicName().toString()) + .setPayloadFormat(format) + .setCustomAttributes(attributes) + .build(); + com.google.cloud.storage.Notification notification = + ctx.getStorage() + .createNotification(state.getBucket().getName(), info); + return state.with(notification); + })) + .build()); + } - private static void list(ArrayList a) {} + private static void list(ArrayList a) { + a.add( + RpcMethodMapping.newBuilder(249, notifications.list) + .withSetup(pubsubTopicSetup) + .withTest( + (ctx, c) -> + ctx.map( + state -> { + List notifications = + ctx.getStorage().listNotifications(state.getBucket().getName()); + return state.with(notifications); + })) + .build()); + } } static final class ObjectAcl { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/State.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/State.java index 94d1542ca..6f24d3486 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/State.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/State.java @@ -29,10 +29,12 @@ import com.google.cloud.storage.CopyWriter; import com.google.cloud.storage.HmacKey; import com.google.cloud.storage.HmacKey.HmacKeyMetadata; +import com.google.cloud.storage.Notification; import com.google.cloud.storage.ServiceAccount; import com.google.cloud.storage.Storage.ComposeRequest; import com.google.common.collect.ImmutableMap; import com.google.errorprone.annotations.Immutable; +import com.google.pubsub.v1.TopicName; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -76,6 +78,10 @@ final class State { private static final Key> KEY_ACLS = new Key<>("acls"); private static final Key KEY_BYTES = new Key<>("bytes"); private static final Key KEY_COMPOSE_REQUEST = new Key<>("composeRequest"); + private static final Key KEY_PUBSUB_TOPIC = new Key<>("topicName"); + private static final Key KEY_NOTIFICATION = new Key<>("notification"); + private static final Key> KEY_LIST_NOTIFICATION = + new Key<>("lise"); private final ImmutableMap, Object> data; @@ -301,6 +307,42 @@ public boolean hasComposeRequest() { return hasValue(KEY_COMPOSE_REQUEST); } + public boolean hasTopicName() { + return hasValue(KEY_PUBSUB_TOPIC); + } + + public TopicName getTopicName() { + return getValue(KEY_PUBSUB_TOPIC); + } + + public State with(TopicName topic) { + return newStateWith(KEY_PUBSUB_TOPIC, topic); + } + + public boolean hasNotification() { + return hasValue(KEY_NOTIFICATION); + } + + public Notification getNotification() { + return getValue(KEY_NOTIFICATION); + } + + public State with(Notification notification) { + return newStateWith(KEY_NOTIFICATION, notification); + } + + public boolean hasNotifications() { + return hasValue(KEY_LIST_NOTIFICATION); + } + + public List getNotifications() { + return getValue(KEY_LIST_NOTIFICATION); + } + + public State with(List notifications) { + return newStateWith(KEY_LIST_NOTIFICATION, notifications); + } + private T getValue(Key key) { Object o = data.get(key); requireNonNull(o, () -> String.format("%s was not found in state", key.name)); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/TestRetryConformance.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/TestRetryConformance.java index 03d671458..0a467256b 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/TestRetryConformance.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/TestRetryConformance.java @@ -69,6 +69,7 @@ final class TestRetryConformance { private final String bucketName2; private final String userProject; private final String objectName; + private final String topicName; private final Supplier lazyHelloWorldUtf8Bytes; private final Path helloWorldFilePath = resolvePathForResource(); @@ -132,6 +133,10 @@ final class TestRetryConformance { String.format( "%s_s%03d-%s-m%03d_obj1", BASE_ID, scenarioId, instructionsString.toLowerCase(), mappingId); + this.topicName = + String.format( + "%s_s%03d-%s-m%03d_top1", + BASE_ID, scenarioId, instructionsString.toLowerCase(), mappingId); lazyHelloWorldUtf8Bytes = Suppliers.memoize( () -> { @@ -256,4 +261,8 @@ private static ServiceAccountCredentials resolveServiceAccountCredentials() { throw new RuntimeException(e); } } + + public String getTopicName() { + return topicName; + } } diff --git a/renovate.json b/renovate.json index 937afbc61..dd9516c91 100644 --- a/renovate.json +++ b/renovate.json @@ -73,7 +73,8 @@ "packagePatterns": [ "^com.google.cloud:google-cloud-pubsub", "^com.google.api.grpc:grpc-google-cloud-kms-v1", - "^com.google.api.grpc:proto-google-cloud-kms-v1" + "^com.google.api.grpc:proto-google-cloud-kms-v1", + "^com.google.api.grpc:proto-google-cloud-pubsub-v1" ], "semanticCommitType": "test", "semanticCommitScope": "deps"