feat: [Spanner] Remove handwritten Retry logic

Spanner/src/Database.php

@@ -17,6 +17,8 @@
 
 namespace Google\Cloud\Spanner;
 
+use Google\ApiCore\RetrySettings;
+use Google\ApiCore\Retrier;
 use Google\ApiCore\Serializer;
 use Google\ApiCore\ValidationException;
 use Google\Cloud\Core\ApiHelperTrait;
@@ -31,7 +33,6 @@
 use Google\Cloud\Core\LongRunning\LROManagerTrait;
 use Google\Cloud\Core\LongRunning\OperationResponseTrait;
 use Google\Cloud\Core\RequestHandler;
-use Google\Cloud\Core\Retry;
 use Google\Cloud\Spanner\Admin\Database\V1\Client\DatabaseAdminClient;
 use Google\Cloud\Spanner\Admin\Database\V1\CreateDatabaseRequest;
 use Google\Cloud\Spanner\Admin\Database\V1\Database\State;
@@ -440,6 +441,10 @@
      *     Configuration Options
      *
      *     @type string[] $statements Additional DDL statements.
+     *     @type RetrySettings|array $retrySettings
+     *           Retry settings to use for this call. Can be a {@see RetrySettings} object, or an
+     *           associative array of retry settings parameters. See the documentation on
+     *           {@see RetrySettings} for example usage.
      * }
      * @return LongRunningOperationManager<Database>
      */
@@ -975,17 +980,16 @@
             return $transaction;
         };
 
-        $delayFn = function (\Exception $e) {
-            if ($e instanceof AbortedException) {
-                return $e->getRetryDelay();
-            }
-            if ($e instanceof ServiceException &&
-                $e->getCode() === Code::INTERNAL &&
-                strpos($e->getMessage(), 'RST_STREAM') !== false
-            ) {
-                return $e->getRetryDelay();
+        $delayFn = function(int $attempts, \Exception $e) {
+            return $e->getRetryDelay()['seconds'] * 1000 + $e->getRetryDelay()['nanos'] / 1000000;
+        };
+        $retryFn = function(\Exception $e, array $options) {
+            if ($e instanceof AbortedException || ($e instanceof ServiceException
+                && $e->getCode() === Code::INTERNAL
+                && strpos($e->getMessage(), 'RST_STREAM') !== false)) {
+                return true;
             }
-            throw $e;
+            return false;
         };
 
         $transactionFn = function ($operation, $session, $options) use ($startTransactionFn) {
@@ -1012,10 +1016,18 @@
             return $res;
         };
 
-        $retry = new Retry($options['maxRetries'], $delayFn);
+        $retrySetting = new RetrySettings([
+            'retriesEnabled' => true,
+            'maxRetries' => $options['maxRetries'],
+            'retryFunction' => $retryFn,
+            'retryDelayFunction' => $delayFn,
+            'maxRetryDelayMillis' => PHP_INT_MAX,
+            'totalTimeoutMillis' => PHP_INT_MAX
+        ]);
+        $retrier = new Retrier($retrySetting);
 
         try {
-            return $retry->execute($transactionFn, [$operation, $session, $options]);
+            return $retrier->execute($transactionFn, [$operation, $session, $options]);
         } finally {
             $session->setExpiration();
         }

Spanner/src/Operation.php

@@ -245,6 +245,7 @@ public function execute(Session $session, $sql, array $options = [])
         // Initially with begin, transactionId will be null.
         // Once transaction is generated, even in the case of stream failure,
         // transaction will be passed to this callable by the Result class.
+        // TODO: Discuss executestreamingsql and streamingRead does NOT have retrySettings
         $call = function ($resumeToken = null, $transaction = null) use (
             $session,
             $sql,
@@ -447,6 +448,7 @@ public function read(
 
         $context = $this->pluck('transactionContext', $options);
 
+        // TODO: Discuss executestreamingsql and streamingRead does NOT have retrySettings
         $call = function ($resumeToken = null, $transaction = null) use (
             $table,
             $session,

Spanner/src/Result.php

@@ -17,8 +17,9 @@
 
 namespace Google\Cloud\Spanner;
 
+use Google\ApiCore\Retry;
+use Google\ApiCore\RetrySettings;
 use Google\Cloud\Core\Exception\ServiceException;
-use Google\Cloud\Core\ExponentialBackoff;
 use Google\Cloud\Spanner\Session\Session;
 use Google\Cloud\Spanner\Session\SessionPoolInterface;
 use Google\Cloud\Spanner\V1\ExecuteSqlRequest\QueryMode;
@@ -51,6 +52,8 @@
     const MODE_PLAN = QueryMode::PLAN;
     const MODE_PROFILE = QueryMode::PROFILE;
 
+    const MAX_RETRY_DELAY_MS = 60000;
+
     /**
      * @var array
      */
@@ -126,6 +129,11 @@
      */
     private $generator;
 
+    /**
+     * @var RetrySettings
+     */
+    private $retrySettings;
+
     /**
      * @param Operation $operation Runs operations against Google Cloud Spanner.
      * @param Session $session The session used for any operations executed.
@@ -150,6 +158,7 @@
         $this->mapper = $mapper;
         $this->retries = $retries;
         $this->createGenerator();
+        $this->retrySettings = $this->getRetrySettings();
     }
 
     /**
@@ -227,13 +236,10 @@
                 $valid = $this->generator->valid();
             } catch (ServiceException $ex) {
                 if ($shouldRetry && $ex->getCode() === Grpc\STATUS_UNAVAILABLE) {
-                    $backoff = new ExponentialBackoff($this->retries, function ($ex) {
-                        return $ex instanceof ServiceException &&
-                            $ex->getCode() === Grpc\STATUS_UNAVAILABLE;
-                    });
+                    $retry = new Retry($this->retrySettings);
                     // Attempt to resume using the last stored resume token and the transaction.
                     // If we successfully resume, flush the buffer.
-                    $this->generator = $backoff->execute($call, [$this->resumeToken, $this->transaction()]);
+                    $this->generator = $retry->execute($call, [$this->resumeToken, $this->transaction]);
                     $bufferedResults = [];
 
                     continue;
@@ -524,13 +530,9 @@
         }
         $call = $this->call;
         $generator = null;
+        $retry = new Retry($this->retrySettings);
 
-        $backoff = new ExponentialBackoff($this->retries, function ($ex) {
-            return $ex instanceof ServiceException &&
-                $ex->getCode() === Grpc\STATUS_UNAVAILABLE;
-        });
-
-        $valid = $backoff->execute(function () use ($call, &$generator) {
+        $valid = $retry->execute(function () use ($call, &$generator) {
             $generator = $call();
             return $generator->valid();
         });
@@ -565,4 +567,20 @@
             }
         }
     }
+
+    private function getRetrySettings()
+    {
+        $retryFn = function(\Exception $e) {
+            return $e instanceof ServiceException &&
+                $e->getCode() === Grpc\STATUS_UNAVAILABLE;
+        };
+        $delayFn = fn($attempts, \Exception $e) => mt_rand(0, 1000) + (pow(2, $attempt) * 1000);
+        return new RetrySettings([
+            'retriesEnabled' => true,   
+            'maxRetries' => $this->retries,
+            'retryFunction' => $retryFn,
+            'delayFunction' => $delayFn,
+            'maxRetryDelayMillis' => self::MAX_RETRY_DELAY_MS
+        ]);
+    }
 }

Spanner/src/SpannerClient.php

@@ -182,6 +182,15 @@ class SpannerClient
      *           request. **Defaults to** `0` with REST and `60` with gRPC.
      *     @type int $retries Number of retries for a failed request. Used only
      *           with default backoff strategy. **Defaults to** `3`.
+     *           TODO: Discuss the need of removing this option. This used to go to the GrpcRequestWrapper and used
+     *           here: https://github.com/googleapis/google-cloud-php/blob/main/Core/src/RequestWrapperTrait.php#L124.
+     *           I dont think it get used in the requestHandler at constructor level, because these options directly been
+     *           send to the V1/Client classes and they expects retry options in the constructor under ClientOptions.
+     *           However, I don't see the use of this maxRetries anywhere. See any V1/Client constructors for more details.
+     *           Furthermore, I see that some APIs does not have the retry capability, eg: SpannerClient.streamingRead and
+     *           settting this at constructor level of requestHandler may not be the correct way. 2 Options:
+     *           1. SpannerClient pass this config to all the APIs internally
+     *           2. Remove maxRetries from here and enable it at API level (eg: see lateset Database.create implementaiton).
      *     @type array $scopes Scopes to be used for the request.
      *     @type string $quotaProject Specifies a user project to bill for
      *           access charges associated with the request.
@@ -203,10 +212,6 @@ class SpannerClient
      *           query execution. Executing a SQL statement with an invalid
      *           optimizer version will fail with a syntax error
      *           (`INVALID_ARGUMENT`) status.
-     *     @type bool $useDiscreteBackoffs `false`: use default backoff strategy
-     *           (retry every failed request up to `retries` times).
-     *           `true`: use discrete backoff settings based on called method name.
-     *           **Defaults to** `false`.
      *     @type array $directedReadOptions Directed read options.
      *           {@see \Google\Cloud\Spanner\V1\DirectedReadOptions}
      *           If using the `replicaSelection::type` setting, utilize the constants available in
@@ -230,25 +235,9 @@ public function __construct(array $config = [])
             'projectIdRequired' => true,
             'hasEmulator' => (bool) $emulatorHost,
             'emulatorHost' => $emulatorHost,
-            'queryOptions' => [],
-            'transportConfig' => [
-                'grpc' => [
-                    // increase default limit to 4MB to prevent metadata exhausted errors
-                    'stubOpts' => ['grpc.max_metadata_size' => 4 * 1024 * 1024,]
-                ]
-            ]
+            'queryOptions' => []
         ];
 
-        # Check this to see it needs to be removed, for now keep it.
-        if (!empty($config['useDiscreteBackoffs'])) {
-            $config = array_merge_recursive($config, [
-                'retries' => 0,
-                'grpcOptions' => [
-                    'retrySettings' => [],
-                ],
-            ]);
-        }
-
         # TODO: Remove the connection related objects
         $this->connection = new Grpc($this->configureAuthentication($config));
         $this->returnInt64AsObject = $config['returnInt64AsObject'];

Spanner/src/Transaction.php

@@ -86,7 +86,8 @@ class Transaction implements TransactionalReadInterface
      * @param Session $session The session to use for spanner interactions.
      * @param string $transactionId [optional] The Transaction ID. If no ID is
      *        provided, the Transaction will be a Single-Use Transaction.
-     * @param bool $isRetry Whether the transaction will automatically retry or not.
+     * @param bool $isRetry Indicates whether the current transaction is
+     *         the result of a retry operation.
      * @param string $tag A transaction tag. Requests made using this transaction will
      *        use this as the transaction tag.
      * @param array $options [optional] {