chore(retry): refactor RetryAlgorithmManager to simplify candidate public api#1108
chore(retry): refactor RetryAlgorithmManager to simplify candidate public api#1108BenWhitehead merged 3 commits intogoogleapis:mainfrom BenWhitehead:ram-refactor
Conversation
…blic api * Remove separate idempotent handling for resumable uploads * Introduce new StorageExceptionHandlerFactory which provides a handle to get exception handlers for idempotent and nonidempotent handlers * Move existing ExceptionHandler implementations to DefaultStorageExceptionHandlerFactory * Refactor StorageOptions to use new StorageExceptionHandlerFactory * Make RetryAlgorithmManager a concrete class implemented in terms of the provided StorageExceptionHandlerFactory rather than an interface * Remove LegacyRetryAlgorithmManager * Remove NewRetryAlgorithmManager
|
|
||
| ExceptionHandler getIdempotentHandler(); | ||
|
|
||
| ExceptionHandler getNonidempotentHandler(); |
There was a problem hiding this comment.
This is much easier to grok compared to having an overload for each operation. Is there potential gotchas to introduce only two calls instead of a call per method? Definitely would like to talk through this more.
Are you thinking of updating the design document with this change so we can discuss / get it approved by folks?
There was a problem hiding this comment.
Yeah, if we think this new approach feels better than I can write it up and share around for feedback from folks.
Compared with the per-method is primarily that there is less granularity. This new approach seems to follow the pattern of the other languages and is probably better for an initial release. Afterward we can gauge whether there is demand to providing the per method approach.
There was a problem hiding this comment.
+1 to @frankyn , this seems easier to understand as a public surface and the loss of granularity/configurability seems minor (and can be worked around by using additional clients if it's really necessary).
google-cloud-storage/src/main/java/com/google/cloud/storage/StorageOptions.java
Outdated
Show resolved
Hide resolved
…ctory * following the precedent set by node use the term strategy for retry selection compared to the generic factory as before * Change retry types from ExceptionHandler to ResultRetryAlgorithm<?>. ExceptionHandler is a subclass of ResultRetryAlgorithm<?>, but this provides us binary compatability moving forward if we ever need to provide an implementation which isn't based on ExceptionHandler. * Remove LegacyStorageExceptionHandlerFactory and replace it with UniformStorageRetryStrategy. Allowing for a single algorithm to be specified and used for both idempotent and non-idempotent cases. * Rename StorageExceptionHandlerFactory to StorageRetryStrategy * Move access of strategy implementations to static method on StorageRetryStrategy.java * Add new getUniformStorageRetryStrategy() which provides a strategy which will use the idempotent algorithm for both cases. * Add javadocs with explanations about the strategies and their behaviors * Add serialVersionUID to each of the new classes which are Serializable
|
I chose the name StorageRetryStrategy for the factory class as it's functioning as an accessor for specific strategies of evaluation (i.e. 'selecting either the idempotent strategy of retrying or the non-idempotent') and because node is using the name IdempotentcyStrategy. If we want to change StorageRetryStrategy to StorageRetryPolicy I can make that change. |
part 1
part 2