feat: Add Retrier class #561

ajupazhamayil · 2024-04-23T15:28:02Z

No description provided.

bshaffer

I cannot track down anywhere the delay config settings are used to delay the retry function. This is something we can look into adding. I'd call it a bug that it's missing, but since it hasn't been implemented yet it's more like a feature.

src/Middleware/RetryMiddleware.php

saranshdhingra · 2024-05-07T10:13:34Z

src/ValidationTrait.php

@@ -56,6 +56,26 @@ public static function validateNotNull(array $arr, array $requiredKeys)
        return self::validateImpl($arr, $requiredKeys, false);
    }

+    /**
+     * @param array $arr Associative array


A description of what this function does will be helpful in the docbloc.

saranshdhingra · 2024-05-07T10:13:57Z

src/RetrySettings.php

-        $this->initialRetryDelayMillis = $settings['initialRetryDelayMillis'];
-        $this->retryDelayMultiplier = $settings['retryDelayMultiplier'];
+        ];
+        $this->validateAllKeysFromOneOf(


Do we mean self:: validateAllKeysFromOneOf. I see it's a static function.

saranshdhingra · 2024-05-07T10:15:06Z

src/RetrySettings.php

@@ -405,6 +461,9 @@ public function with(array $settings)
            'noRetriesRpcTimeoutMillis' => $this->getNoRetriesRpcTimeoutMillis(),
            'maxRetries' => $this->getMaxRetries(),
            'retryFunction' => $this->getRetryFunction(),
+            'retryDelayFunction' => $this->getRetryDelayFunction(),


I thought we had the general agreement that we want to remove the delayFunction and only give option to add floats?

What if the delay needs to be retrieved from the server exception? Thats a usecase for the Spanner

saranshdhingra · 2024-05-07T10:17:28Z

src/Retrier.php

+     * @return mixed
+     * @throws \Exception
+     */
+    public function execute(callable $call, array $options)


This is certainly something new, but do we want to add the ability to change the $call or $options after a failed attempt.

We had this ability in Core where we allowed changing of headers using retryListeners.
Although we don't need this functionality right now, but I just wanted to initiate a discussion on this.

Yeah, I did not add this since it was not a requirement right now.

bshaffer

I feel like the design here is slightly off - there's still too much logic happening in RetryMiddleware - it should be as thin as possible. Also, having the $retryAttempts as part of RetrySettings is not what we want (think of RetrySettings as an object that should be able to be passed around to different RPC calls without unexpected behavior).

bshaffer · 2024-05-07T16:52:10Z

src/RetrySettings.php

+     * The number of retries that have already been attempted.
+     * The original API call will have $retryAttempts set to 0.
+     */
+    private int $retryAttempts;


This object represents the settings for retrying, e.g. HOW the retrying will be performed.

$retryAttempts is related to the actual retrying taking place. This should be handled by the Retrier, and NOT the RetrySettings.

bshaffer · 2024-05-07T16:54:17Z

src/Middleware/RetryMiddleware.php

     *
     * @return PromiseInterface
     * @throws ApiException
     */
-    private function retry(Call $call, array $options, string $status)
+    private function retry(Call $call, array $options, ApiException $exception)


Shouldn't this entire function live in the Retrier? That would also remove the need for so many public methods in the Retrier.

@bshaffer This function uses GuzzleHttp\Promise\PromiseInterface. In our initial discussion, we wanted the Retrier to be as generic as possible. Thats why I did not move this functionality to the Retrier class.

Guzzle's PromiseInterface is pretty generic (see here). Although the package has "http" in the name (guzzlehttp/promises), the interface has nothing to do with HTTP requests. I think it makes more sense to continue using the PromiseInterface as the return type for the Retrier.

If you don't like this, the other option is to do what we did for ExponentialBackoff, and wrap the executable in a try/catch inside the Retrier.

I prefer the PromiseInterface approach because it's already implemented, and we won't have to refactor it and run the risk of unexpected behavior. However, what we have here is the Retrier and RetrierMiddleware calling each other in a somewhat convoluted circle, which makes it hard for me to follow what each one's purpose is.

bshaffer · 2024-05-09T14:32:39Z

src/Retrier.php

+     * @return void
+     * @throws ApiException
+     */
+    public function checkDeadlineExceeded(float $currentTimeMillis)


This seems like something that the retrier should check internally, and shouldn't need to be a public method

This made public to use it by the RetryMiddleware

bshaffer · 2024-05-09T14:32:58Z

src/Retrier.php

+     *
+     * @return float
+     */
+    public function calculateRetryDeadlineMillis(float $currentTimeMillis)


This seems like something the retrier should be able to calculate internally, and that it shouldn't need to be a public method.

This made public to use it by the RetryMiddleware

bshaffer · 2024-05-09T14:36:44Z

src/RetrySettings.php

+    {
+        if ($this->retryDelayFunction) {
+            return min(
+                call_user_func($this->retryDelayFunction, $this->retryAttempts, $exception),


Why are we passing in the $exception in order to determine the length of the retry delay?

Yes, sometimes the exception contains the delay. See here: https://github.com/googleapis/google-cloud-php/blob/076287b03e801bfaff4f43809de8c4bfc7c9e5d6/Spanner/src/Database.php#L916

bshaffer · 2024-05-09T14:50:01Z

src/Middleware/RetryMiddleware.php

+     * @param int $deadlineMs TODO: Deprecate if possible.
+     * @param int $retryAttempts TODO: Deprecate if possible.


If these need to be deprecated, we should deprecate them as part of this PR.

bshaffer · 2024-05-09T14:52:03Z

src/RetrySettings.php

@@ -264,37 +281,72 @@ class RetrySettings
     *                    This option is experimental.
     *     @type callable $retryFunction This function will be used to decide if we should retry or not.
     *                    Callable signature: `function (Exception $e, array $options): bool`
-     *                    This option is experimental.
+     *     @type callable $retryDelayFunction Optional. This function will be used to decide the delay between retries.


nit: line lengths

Suggested change

* @type callable $retryDelayFunction Optional. This function will be used to decide the delay between retries.

* @type callable $retryDelayFunction Optional. This function will be used to decide the

* delay between retries.

bshaffer · 2024-05-09T14:54:49Z

src/RetrySettings.php

+            [
+                'retryDelayFunction',
+            ]
+        ];


If a user supplies settings which are invalid, we should throw an exception, instead of having hidden logic which they have to read the code to understand.

if (!emtpy($settings['initialRetryDelayMillis']) && !emtpy($settings['retryDelayFunction'])) { throw new InvalidArgumentException('Cannot supply both "retryableCodes" and "retryFunction" to RetrySettings'); }

bshaffer · 2024-05-09T14:56:19Z

src/ValidationTrait.php

+     * @return array Returns $arr for fluent use
+     * @throws ValidationException
+     */
+    public static function validateAllKeysFromOneOf(array $arr, array $requiredKeys1, array $requiredKeys2)


A function like this is a bit over engineered - The logic should be clearly defined and readable. The retrySettings object should remain simple - we provide it with settings and it tells us what those settings are.

bshaffer · 2024-05-09T15:14:53Z

src/Middleware/RetryMiddleware.php

     *
     * @return PromiseInterface
     * @throws ApiException
     */
-    private function retry(Call $call, array $options, string $status)
+    private function retry(Call $call, array $options, ApiException $exception)


Guzzle's PromiseInterface is pretty generic (see here). Although the package has "http" in the name (guzzlehttp/promises), the interface has nothing to do with HTTP requests. I think it makes more sense to continue using the PromiseInterface as the return type for the Retrier.

If you don't like this, the other option is to do what we did for ExponentialBackoff, and wrap the executable in a try/catch inside the Retrier.

I prefer the PromiseInterface approach because it's already implemented, and we won't have to refactor it and run the risk of unexpected behavior. However, what we have here is the Retrier and RetrierMiddleware calling each other in a somewhat convoluted circle, which makes it hard for me to follow what each one's purpose is.

bshaffer · 2024-05-09T15:26:21Z

src/RetrySettings.php

+    /**
+     * The deadline in milliseconds.
+     */
+    private ?float $deadlineMillis;


I'm surprised this is a float, since milliseconds are an integer.

I have followed this from RetryMiddleware here:

gax-php/src/Middleware/RetryMiddleware.php

Line 48 in b4e80d6

private ?float $deadlineMs;

bshaffer · 2024-05-09T21:04:31Z

src/Middleware/RetryMiddleware.php

+            if (!$retrySettings->retriesEnabled() && $retrySettings->getNoRetriesRpcTimeoutMillis() > 0) {
+                $options['timeoutMillis'] = $retrySettings->getNoRetriesRpcTimeoutMillis();
+            } elseif ($retrySettings->getInitialRpcTimeoutMillis() > 0) {
+                $options['timeoutMillis'] = $retrySettings->getInitialRpcTimeoutMillis();


After discussing with @saranshdhingra, I think having a parameterUpdateFunction as part of either Retrier or or RetrySettings is the way to handle this - we need to update this on every call, but only for calls made through RetryMiddleware, so it should be generic.

I have a WIP here: 6a94036

ajupazhamayil · 2024-05-21T15:16:25Z

We have decided not to go with gax Retrier logic, Instead we will continue to use the existing core's Retry logic. Hence closing this.

feat: Add Retrier class

1f50a86

ajupazhamayil mentioned this pull request Apr 24, 2024

feat: Add Retry class #560

Closed

Add logic to Retrier class

e30f44e

ajupazhamayil force-pushed the gax-retry-2 branch from fd4c45b to e30f44e Compare April 30, 2024 12:04

ajupazhamayil added 4 commits April 30, 2024 12:07

resolve style issues

460910e

Remove future todos to focus

eeaed0d

retracting the retryFunction signature change

c8d1e5e

resolve PR test errors

cd7d86c

bshaffer reviewed Apr 30, 2024

View reviewed changes

src/Middleware/RetryMiddleware.php Outdated Show resolved Hide resolved

RetryMiddleware::$deadlineMillis -> RetryMiddleware::$deadlineMs

a1c4fbb

bshaffer mentioned this pull request May 3, 2024

feat: [Spanner] Remove handwritten Retry logic googleapis/google-cloud-php#7235

Closed

saranshdhingra reviewed May 7, 2024

View reviewed changes

bshaffer reviewed May 7, 2024

View reviewed changes

bshaffer reviewed May 9, 2024

View reviewed changes

ajupazhamayil closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Retrier class #561

feat: Add Retrier class #561

ajupazhamayil commented Apr 23, 2024

bshaffer left a comment

saranshdhingra May 7, 2024

saranshdhingra May 7, 2024

saranshdhingra May 7, 2024

ajupazhamayil May 7, 2024

saranshdhingra May 7, 2024

ajupazhamayil May 7, 2024

bshaffer left a comment

bshaffer May 7, 2024 •

edited

Loading

bshaffer May 7, 2024

ajupazhamayil May 9, 2024 •

edited

Loading

bshaffer May 9, 2024

bshaffer May 9, 2024

ajupazhamayil May 10, 2024

bshaffer May 9, 2024

ajupazhamayil May 10, 2024

bshaffer May 9, 2024

ajupazhamayil May 10, 2024

bshaffer May 9, 2024

bshaffer May 9, 2024

bshaffer May 9, 2024

bshaffer May 9, 2024

bshaffer May 9, 2024

bshaffer May 9, 2024

ajupazhamayil May 10, 2024

bshaffer May 9, 2024

ajupazhamayil commented May 21, 2024

		* @param int $deadlineMs TODO: Deprecate if possible.
		* @param int $retryAttempts TODO: Deprecate if possible.

	* @type callable $retryDelayFunction Optional. This function will be used to decide the delay between retries.
	* @type callable $retryDelayFunction Optional. This function will be used to decide the
	* delay between retries.

feat: Add Retrier class #561

feat: Add Retrier class #561

Conversation

ajupazhamayil commented Apr 23, 2024

bshaffer 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

bshaffer left a comment

Choose a reason for hiding this comment

bshaffer May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajupazhamayil May 9, 2024 • edited Loading

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

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

ajupazhamayil commented May 21, 2024

bshaffer May 7, 2024 •

edited

Loading

ajupazhamayil May 9, 2024 •

edited

Loading