Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add Retrier class #561

Closed
wants to merge 7 commits into from
Closed

feat: Add Retrier class #561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f50a86
feat: Add Retrier class
ajupazhamayil Apr 23, 2024
e30f44e
Add logic to Retrier class
ajupazhamayil Apr 23, 2024
460910e
resolve style issues
ajupazhamayil Apr 30, 2024
eeaed0d
Remove future todos to focus
ajupazhamayil Apr 30, 2024
c8d1e5e
retracting the retryFunction signature change
ajupazhamayil Apr 30, 2024
cd7d86c
resolve PR test errors
ajupazhamayil Apr 30, 2024
a1c4fbb
RetryMiddleware::$deadlineMillis -> RetryMiddleware::$deadlineMs
ajupazhamayil May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 58 additions & 86 deletions src/Middleware/RetryMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use Google\ApiCore\ApiException;
use Google\ApiCore\ApiStatus;
use Google\ApiCore\Call;
use Google\ApiCore\Retrier;
use Google\ApiCore\RetrySettings;
use GuzzleHttp\Promise\PromiseInterface;

Expand All @@ -44,25 +45,26 @@ class RetryMiddleware implements MiddlewareInterface
{
/** @var callable */
private $nextHandler;
private RetrySettings $retrySettings;
private ?float $deadlineMs;
private Retrier $retrier;

/*
* The number of retries that have already been attempted.
* The original API call will have $retryAttempts set to 0.
/**
* @param callable $nextHandler
* @param RetrySettings $retrySettings
* @param int $deadlineMs TODO: Deprecate if possible.
* @param int $retryAttempts TODO: Deprecate if possible.
Comment on lines +53 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
private int $retryAttempts;

public function __construct(
callable $nextHandler,
RetrySettings $retrySettings,
$deadlineMs = null,
$retryAttempts = 0
) {
$this->nextHandler = $nextHandler;
$this->retrySettings = $retrySettings;
$this->deadlineMs = $deadlineMs;
$this->retryAttempts = $retryAttempts;
$retrySettings = $retrySettings->with([
'deadlineMillis' => $deadlineMs,
'retryAttempts' => $retryAttempts
]);
$this->retrier = new Retrier($retrySettings);
}

/**
Expand All @@ -74,121 +76,91 @@ public function __construct(
public function __invoke(Call $call, array $options)
{
$nextHandler = $this->nextHandler;
$retrySettings = $this->retrier->getRetrySettings();

if (!isset($options['timeoutMillis'])) {
// default to "noRetriesRpcTimeoutMillis" when retries are disabled, otherwise use "initialRpcTimeoutMillis"
if (!$this->retrySettings->retriesEnabled() && $this->retrySettings->getNoRetriesRpcTimeoutMillis() > 0) {
$options['timeoutMillis'] = $this->retrySettings->getNoRetriesRpcTimeoutMillis();
} elseif ($this->retrySettings->getInitialRpcTimeoutMillis() > 0) {
$options['timeoutMillis'] = $this->retrySettings->getInitialRpcTimeoutMillis();
if (!$retrySettings->retriesEnabled() && $retrySettings->getNoRetriesRpcTimeoutMillis() > 0) {
$options['timeoutMillis'] = $retrySettings->getNoRetriesRpcTimeoutMillis();
} elseif ($retrySettings->getInitialRpcTimeoutMillis() > 0) {
$options['timeoutMillis'] = $retrySettings->getInitialRpcTimeoutMillis();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

// Call the handler immediately if retry settings are disabled.
if (!$this->retrySettings->retriesEnabled()) {
if (!$retrySettings->retriesEnabled()) {
return $nextHandler($call, $options);
}

return $nextHandler($call, $options)->then(null, function ($e) use ($call, $options) {
$retryFunction = $this->getRetryFunction();

// If the number of retries has surpassed the max allowed retries
// then throw the exception as we normally would.
// If the maxRetries is set to 0, then we don't check this condition.
if (0 !== $this->retrySettings->getMaxRetries()
&& $this->retryAttempts >= $this->retrySettings->getMaxRetries()
) {
throw $e;
}
// If the retry function returns false then throw the
// exception as we normally would.
if (!$retryFunction($e, $options)) {
throw $e;
return $nextHandler($call, $options)->then(null, function ($exception) use ($call, $options) {
if (!$this->retrier->isRetryable($exception)) {
throw $exception;
}

// Retry function returned true, so we attempt another retry
return $this->retry($call, $options, $e->getStatus());
return $this->retry($call, $options, $exception);
});
}

/**
* @param Call $call
* @param array $options
* @param string $status
* @param ApiException $exception
*
* @return PromiseInterface
* @throws ApiException
*/
private function retry(Call $call, array $options, string $status)
private function retry(Call $call, array $options, ApiException $exception)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ajupazhamayil ajupazhamayil May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
$delayMult = $this->retrySettings->getRetryDelayMultiplier();
$maxDelayMs = $this->retrySettings->getMaxRetryDelayMillis();
$timeoutMult = $this->retrySettings->getRpcTimeoutMultiplier();
$maxTimeoutMs = $this->retrySettings->getMaxRpcTimeoutMillis();
$totalTimeoutMs = $this->retrySettings->getTotalTimeoutMillis();

$delayMs = $this->retrySettings->getInitialRetryDelayMillis();
$timeoutMs = $options['timeoutMillis'];
$currentTimeMs = $this->getCurrentTimeMs();
$deadlineMs = $this->deadlineMs ?: $currentTimeMs + $totalTimeoutMs;

if ($currentTimeMs >= $deadlineMs) {
throw new ApiException(
'Retry total timeout exceeded.',
\Google\Rpc\Code::DEADLINE_EXCEEDED,
ApiStatus::DEADLINE_EXCEEDED
);
}

$delayMs = min($delayMs * $delayMult, $maxDelayMs);
$timeoutMs = (int) min(
$timeoutMs * $timeoutMult,
$maxTimeoutMs,
$deadlineMs - $this->getCurrentTimeMs()
$currentTime = $this->retrier->getCurrentTimeMillis();
$this->retrier->checkDeadlineExceeded($currentTime);
$deadlineMillis = $this->retrier->calculateRetryDeadlineMillis($currentTime);
$retrySettings = $this->retrier->getRetrySettings()->with([
'retryAttempts' => $this->retrier->getRetrySettings()->getRetryAttempts() + 1,
'deadlineMillis' => $deadlineMillis
]);
$timeout = $this->calculateTimeoutMillis(
$retrySettings,
$options['timeoutMillis']
);

$nextHandler = new RetryMiddleware(
$this->nextHandler,
$this->retrySettings->with([
'initialRetryDelayMillis' => $delayMs,
]),
$deadlineMs,
$this->retryAttempts + 1
$retrySettings,
$retrySettings->getDeadlineMillis(),
$retrySettings->getRetryAttempts()
);

// Set the timeout for the call
$options['timeoutMillis'] = $timeoutMs;
$options['timeoutMillis'] = $timeout;

return $nextHandler(
$call,
$options
);
}

protected function getCurrentTimeMs()
{
return microtime(true) * 1000.0;
}

/**
* This is the default retry behaviour.
* Calculates the timeout for the call.
*
* @param RetrySettings $retrySettings
* @param int $timeoutMillis
*
* @return int
*/
private function getRetryFunction()
{
return $this->retrySettings->getRetryFunction() ??
function (\Throwable $e, array $options): bool {
// This is the default retry behaviour, i.e. we don't retry an ApiException
// and for other exception types, we only retry when the error code is in
// the list of retryable error codes.
if (!$e instanceof ApiException) {
return false;
}

if (!in_array($e->getStatus(), $this->retrySettings->getRetryableCodes())) {
return false;
}

return true;
};
private function calculateTimeoutMillis(
RetrySettings $retrySettings,
int $timeoutMillis
) {
$maxTimeoutMillis = $retrySettings->getMaxRpcTimeoutMillis();
$timeoutMult = $retrySettings->getRpcTimeoutMultiplier();
$deadlineMillis = $retrySettings->getDeadlineMillis();
$currentTime = $this->retrier->getCurrentTimeMillis();

return (int) min(
$timeoutMillis * $timeoutMult,
$maxTimeoutMillis,
$deadlineMillis - $currentTime
);
}
}
172 changes: 172 additions & 0 deletions src/Retrier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
<?php
/*
* Copyright 2024 Google LLC
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google Inc. nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
namespace Google\ApiCore;

use Google\ApiCore\ApiException;
use Google\ApiCore\ApiStatus;
use Google\ApiCore\RetrySettings;

/**
* Retrier functionality.
*/
class Retrier
{
private RetrySettings $retrySettings;

public function __construct(
RetrySettings $retrySettings
) {
$this->retrySettings = $retrySettings;
}

/**
* @return RetrySettings
*/
public function getRetrySettings()
{
return $this->retrySettings;
}

/**
* Execute the callable with the retry logic.
*
* @param callable $call
* @param array $options
*
* @return mixed
* @throws \Exception
*/
public function execute(callable $call, array $options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
try {
return call_user_func_array($call, $options);
} catch (\Exception $exception) {
if (!$this->isRetryable($exception)) {
throw $exception;
}
// Check if the deadline has already been exceeded.
$this->checkDeadlineExceeded($this->getCurrentTimeMillis());
$this->retrySettings = $this->retrySettings->with([
'retryAttempts' => $this->retrySettings->getRetryAttempts() + 1
]);
$retryDelayMillis = $this->retrySettings->getRetryDelayMillis($exception);
// Millis to Micro conversion.
usleep($retryDelayMillis * 1000);
}
$this->execute($call, $options);
}

/**
* @param \Exception $exception
*
* @return bool
*/
public function isRetryable(\Exception $exception, array $options = [])
{
$retryFunction = $this->getRetryFunction();

// Don't retry if the number of retries has surpassed the max allowed retries.
// If the maxRetries is set to 0, then we don't check this condition.
if (0 !== $this->retrySettings->getMaxRetries()
&& $this->retrySettings->getRetryAttempts() >= $this->retrySettings->getMaxRetries()
) {
return false;
}
// Don't retry if the retry function returns false.
return $retryFunction($exception, $options);
}

/**
* @param float $currentTimeMillis
*
* @return void
* @throws ApiException
*/
public function checkDeadlineExceeded(float $currentTimeMillis)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made public to use it by the RetryMiddleware

{
$deadlineMillis = $this->calculateRetryDeadlineMillis($currentTimeMillis);

if ($currentTimeMillis >= $deadlineMillis) {
throw new ApiException(
'Retry total timeout exceeded.',
\Google\Rpc\Code::DEADLINE_EXCEEDED,
ApiStatus::DEADLINE_EXCEEDED
);
}
}

/**
* @param float $currentTimeMillis
*
* @return float
*/
public function calculateRetryDeadlineMillis(float $currentTimeMillis)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made public to use it by the RetryMiddleware

{
if ($this->retrySettings->getDeadlineMillis()) {
return $this->retrySettings->getDeadlineMillis();
}
$totalTimeoutMillis = $this->retrySettings->getTotalTimeoutMillis();
return $currentTimeMillis + $totalTimeoutMillis;
}

/**
* @return float
*/
public function getCurrentTimeMillis()
{
return microtime(true) * 1000.0;
}

/**
* This is the default retry behaviour.
*
* @return callable
*/
private function getRetryFunction()
{
return $this->retrySettings->getRetryFunction() ??
function (\Throwable $e, array $options = []): bool {
// This is the default retry behaviour, i.e. we don't retry an ApiException
// and for other exception types, we only retry when the error code is in
// the list of retryable error codes.
if (!$e instanceof ApiException) {
return false;
}

if (!in_array($e->getStatus(), $this->retrySettings->getRetryableCodes())) {
return false;
}

return true;
};
}
}
Loading