chore(retry): introduce RetryAlgorithmManager #1029

BenWhitehead · 2021-09-20T20:38:25Z

Summary

RetryAlgorithmManager provides an abstraction point for determining
which ExceptionHandler should be used for a specific call.
Implement LegacyRetryAlgorithmManager for which all methods return
BaseService.EXCEPTION_HANDLER which is hardcoded everywhere
previously.
Stub out NewRetryAlgorithmManager which currently extends
LegacyRetryAlgorithmManager, but allows for overriding individual
methods in an incremental manner.

Refactoring

All usages of BaseService.EXCEPTION_HANDLER in StorageImpl, Blob,
Bucket etc now instead use RetryAlgorithmManager to resolve a
handler per method.
Refactor Blob to use Retrying#run instead of runWithRetries
Split starting of a resumable upload session out of BlobWriteChannel
to ResumableMedia. This decouple the ability to instantiate a
BlobWriteChannel independent of performing an RPC to resolve the
uploadId. This split is also necessary to allow differing configuration
of ExceptionHandler for open vs write of a resumable upload session.
Refactor StorageException#translateAndThrow to allow for an option which
doesn't throw, new StorageException#coalesce which attempts to coalesce
to a BaseServiceException but doesn't throw.
Relax several easymock assertions on StorageOptions method. (Calling get
on an immutable object shouldn't be so concerned with invocation count)

Related to #1024

### Summary * RetryAlgorithmManager provides an abstraction point for determining which ExceptionHandler should be used for a specific call. * Implement LegacyRetryAlgorithmManager for which all methods return BaseService.EXCEPTION_HANDLER which is hardcoded everywhere previously. * Stub out NewRetryAlgorithmManager which currently extends LegacyRetryAlgorithmManager, but allows for overriding individual methods in an incremental manner. ### Refactoring * All usages of BaseService.EXCEPTION_HANDLER in StorageImpl, Blob, Bucket etc now instead use RetryAlgorithmManager to resolve a handler per method. * Refactor Blob to use Retrying#run instead of runWithRetries * Split starting of a resumable upload session out of BlobWriteChannel to ResumableMedia. This decouple the ability to instantiate a BlobWriteChannel independent of performing an RPC to resolve the uploadId. This split is also necessary to allow differing configuration of ExceptionHandler for open vs write of a resumable upload session. * Refactor StorageException#translateAndThrow to allow for an option which doesn't throw, new StorageException#coalesce which attempts to coalesce to a BaseServiceException but doesn't throw. * Relax several easymock assertions on StorageOptions method. (Calling get on an immutable object shouldn't be so concerned with invocation count) Related to #1024

frankyn

Still reading through the PR, have one leading question on RetryAlgorithmManager definition

frankyn · 2021-09-22T21:34:15Z

google-cloud-storage/src/main/java/com/google/cloud/storage/RetryAlgorithmManager.java

+import java.util.List;
+import java.util.Map;
+
+interface RetryAlgorithmManager extends Serializable {


Why not abstract instead of interface, given additional API operations may be added in the future?

Abstract classes aren't mockable without some additional hackery necessary, and a class can only extend one class but can implement multiple interfaces. Theoretically someone might want to provide an implementation which extends their own internal class while also implementing this interface.

We can provide a Delegating implementation which can ensure not needing to re-implement the whole interface to override a single method, while keeping the public usage api against an interface.

Naive question: It's a breaking change if a consumer chooses to implement the interface and we add an additional method which would require them to implement the method. We've hit conversation before with Storage interface and the recommendation at that time was to use class's. In this case we are accepting that breaking change, but that doesn't follow SemVer. am I understanding this correctly?

That's fair, my intent with the interface/delegate approach was to provide the interface always, with the convenience of extending a class through the delegate. We don't want people to directly extend our internal implementations as that binds those implementations into the public api preventing us from making any modifications we might need to down the road.

Theoretically, we could provide default implementations in the interface now that we have java 8, but I'm not totally sure we want to do that.

Side note, this interface is currently package private and not part of the public api. If we want we can poke ahead with the interface and see how it goes before deciding to expose it publicly.

tritone

Have a few questions; I'm going to lean on @frankyn and @JesseLovelace to give this a deeper review.

tritone · 2021-09-23T03:08:12Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

-    } catch (RetryHelper.RetryHelperException e) {
-      StorageException.translateAndThrow(e);
-    }
+    ExceptionHandler exceptionHandler = retryAlgorithmManager.getForObjectsGet(pb, requestOptions);


Can you explain why we need a separate exception handler getter for each API method? Naively, I would think it would be possible to do something like what python has (providing one retryer for always-idempotent ops and one for each type of conditional retry; see https://googleapis.dev/python/storage/latest/retry_timeout.html#module-google.cloud.storage.retry ). Is it possible to abstract something at a similar level here?

Compared to python we don't have the pattern of the retry algorithm provided as a method argument, this approach allows for per rpc method handling. In our default implementation we'll most likely end up with common ExceptionHandlers which can simply be returned when applicable.

Part of this api is also functioning as a bit of a prototype of what we might want the public api to look like. In java-core there is a pattern of being able to configure retry settings per rpc method as there is not necessarily the idempotency buckets available universally.

Sounds good. So do you think we'll be able to get more conciseness in the public API? Or probably not?

Possibly, we'll have to weigh it against how gax-java allows configuration of retry settings as I know there is some ability for per rpc config in their public api.

google-cloud-storage/src/test/java/com/google/cloud/storage/ResumableMediaTest.java

google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java

frankyn

Overall this LGTM, I have a few nits/questions. I'm really curious about the design before approving.

frankyn · 2021-09-24T16:54:25Z

google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java

@@ -2148,16 +2146,6 @@ public void testRuntimeException() {
    }
  }

-  @Test
-  public void testWriterWithSignedURL() throws MalformedURLException {


Why is this being removed?

Ignore. It got refactored missed that.

The actual testing guts were moved to ResumableMediaTest.java and given a more descriptive name here: https://github.com/googleapis/java-storage/pull/1029/files#diff-3d1e0f94d16d08b59c904d3e772743afad8827221342f4d2a13624fcec1ab0ccR34-R45

Signed URL handling is only necessary for the creation of the session but has no bearing on the operation of the BlobWriteChannel itself.

frankyn · 2021-09-24T16:58:35Z

google-cloud-storage/src/test/java/com/google/cloud/storage/BlobTest.java

    BlobInfo target = BlobInfo.newBuilder(BlobId.of("bt", "nt")).build();
    BlobId targetId = BlobId.of("bt", "nt");
    CopyWriter copyWriter = createMock(CopyWriter.class);
    Capture<CopyRequest> capturedCopyRequest = Capture.newInstance();
-    expect(storage.getOptions()).andReturn(mockOptions);
+    expect(storage.getOptions()).andReturn(mockOptions).anyTimes();


Could you clarify the rationale for introducing anyTimes()?

Ben in description of PR:

Relax several easymock assertions on StorageOptions method. (Calling get on an immutable object shouldn't be so concerned with invocation count)

With the introduction of the RetryAlgorithmManager to StorageOptions, the number of times there are interactions with storage to get StorageOptions, or calls against an instance of StorageOptions increased. As far as test maintenence is concerned N calls to a method on an immutable object and produces an immutable result aren't interesting from a number of invocations stand point. So, rather than fighting with the numbers being unexpected, I opted to remove the constraint on the number of times the method is allowed to be called.

Makes perfect sense, thanks for clarifying the change.

frankyn · 2021-09-24T17:01:32Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

+        retryAlgorithmManager.getForResumableUploadSessionCreate(
+            Collections
+                .emptyMap()); // TODO: is it possible to know if a signed url is configured to have
+    // a constraint which makes it idempotent?


o, this is a really good call out. You can in theory generate many sessions with one signed URL IIRC. As far as making it idempotent, yes, you can use precondition headers for XML API: https://cloud.google.com/storage/docs/xml-api/put-object#request-headers

frankyn · 2021-09-24T17:11:19Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

-    return run(() -> storageRpc.load(storageObject, optionsMap), Function.identity());
+    ExceptionHandler exceptionHandler =
+        retryAlgorithmManager.getForObjectsGet(storageObject, optionsMap);
+    return run(


Question on design, I left a comment on the design document. I'm wondering if we can reduce complexity here a little bit for future us.

Update: @BenWhitehead and I spoke on Friday through the current design and decided to move forward with the existing design for the following reasons:

It's not explicitly exposed as a Public API (yet).

It refactors the complexity of Resumable Media operations to map back to gRPC API spec which helps clean up internal maintanence

We would like to make progress towards Retry Strategy alignment while acknowledging there's not enough information on gotchas from introducing gRPC Retries yet. We are making decisions with the information we have right now.

BenWhitehead requested a review from a team as a code owner September 20, 2021 20:38

product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Sep 20, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 20, 2021

BenWhitehead requested review from frankyn and JesseLovelace September 21, 2021 19:06

frankyn reviewed Sep 22, 2021

View reviewed changes

tritone reviewed Sep 23, 2021

View reviewed changes

frankyn suggested changes Sep 24, 2021

View reviewed changes

frankyn approved these changes Sep 27, 2021

View reviewed changes

BenWhitehead merged commit ea1eeb9 into googleapis:master Sep 27, 2021

BenWhitehead deleted the smarter-retries branch September 27, 2021 17:23

BenWhitehead added this to the Retry Conformance Project milestone Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(retry): introduce RetryAlgorithmManager #1029

chore(retry): introduce RetryAlgorithmManager #1029

Uh oh!

BenWhitehead commented Sep 20, 2021

frankyn left a comment

frankyn Sep 22, 2021

BenWhitehead Sep 22, 2021

frankyn Sep 22, 2021 •

edited

Loading

BenWhitehead Sep 22, 2021 •

edited

Loading

tritone left a comment

tritone Sep 23, 2021

BenWhitehead Sep 23, 2021

tritone Sep 23, 2021

BenWhitehead Sep 24, 2021

Uh oh!

Uh oh!

frankyn left a comment

frankyn Sep 24, 2021

frankyn Sep 24, 2021

BenWhitehead Sep 24, 2021

frankyn Sep 24, 2021

frankyn Sep 24, 2021

BenWhitehead Sep 24, 2021

frankyn Sep 24, 2021

frankyn Sep 24, 2021

frankyn Sep 24, 2021

frankyn Sep 27, 2021

chore(retry): introduce RetryAlgorithmManager #1029

chore(retry): introduce RetryAlgorithmManager #1029

Uh oh!

Conversation

BenWhitehead commented Sep 20, 2021

Summary

Refactoring

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

BenWhitehead Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn Sep 22, 2021 •

edited

Loading

BenWhitehead Sep 22, 2021 •

edited

Loading