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.

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

chore(gcs-conformance): remove public methods from RetryTrait #6075

Merged
merged 4 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 5 additions & 57 deletions Storage/src/Connection/RetryTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

namespace Google\Cloud\Storage\Connection;

use Google\Cloud\Storage\StorageClient;

/**
* Trait which provides helper methods for retry logic.
*
Expand Down Expand Up @@ -96,29 +98,7 @@ trait RetryTrait
* - Retrying only when the operation is considered idempotent(default).
* These configurations are supplied for per api call basis.
*
* We can set $options['retryStrategy'] to one of "always", "never" and
* "idempotent". Anything apart from them is considered as "idempotent" and will be
* retried as intended.
*/

/**
* Retry an API operation when an exception occurs if the exception has a retryable error code.
* @var string
*/
private static $RETRY_STRATEGY_ALWAYS = 'always';

/**
* Never retry an API operation.
* @var string
*/
private static $RETRY_STRATEGY_NEVER = 'never';

/**
* Retry an API operation only if it is considered idempotent
* and the exception has a retryable error code.
* @var string
*/
private static $RETRY_STRATEGY_IDEMPOTENT = 'idempotent';

/**
* Header that identifies a specific request hash. The
Expand Down Expand Up @@ -151,7 +131,7 @@ private function getRestRetryFunction($resource, $method, array $args)
$preconditionSupplied = $this->isPreConditionSupplied($methodName, $args);
$retryStrategy = isset($args['retryStrategy']) ?
$args['retryStrategy'] :
self::$RETRY_STRATEGY_IDEMPOTENT;
StorageClient::RETRY_IDEMPOTENT;

return function (
\Exception $exception
Expand Down Expand Up @@ -217,7 +197,7 @@ private function retryDeciderFunction(
$preconditionSupplied,
$retryStrategy
) {
if ($retryStrategy == self::$RETRY_STRATEGY_NEVER) {
if ($retryStrategy == StorageClient::RETRY_NEVER) {
return false;
}

Expand All @@ -228,7 +208,7 @@ private function retryDeciderFunction(
// idempotent with preconditions supplied.

if (in_array($statusCode, self::$httpRetryCodes)) {
if ($retryStrategy == self::$RETRY_STRATEGY_ALWAYS) {
if ($retryStrategy == StorageClient::RETRY_ALWAYS) {
return true;
} elseif ($isIdempotent) {
return true;
Expand All @@ -240,38 +220,6 @@ private function retryDeciderFunction(
return false;
}

/**
* Getter function that returns the RETRY_STRATEGY_ALWAYS key.
*/
public static function getStrategyAlwaysKey()
saranshdhingra marked this conversation as resolved.
Show resolved Hide resolved
{
return self::$RETRY_STRATEGY_ALWAYS;
}

/**
* Getter function that returns the RETRY_STRATEGY_NEVER key.
*/
public static function getStrategyNeverKey()
{
return self::$RETRY_STRATEGY_NEVER;
}

/**
* Getter function that returns the RETRY_STRATEGY_IDEMPOTENT key.
*/
public static function getStrategyIdempotentKey()
{
return self::$RETRY_STRATEGY_IDEMPOTENT;
}

/**
* Getter function that returns the HTTP retry codes.
*/
public static function getHttpRetryCodes()
{
return self::$httpRetryCodes;
}

/**
* Utility func that returns the list of headers that need to be
* attached to every request and its retries.
Expand Down
18 changes: 18 additions & 0 deletions Storage/src/StorageClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,24 @@ class StorageClient
const READ_ONLY_SCOPE = 'https://www.googleapis.com/auth/devstorage.read_only';
const READ_WRITE_SCOPE = 'https://www.googleapis.com/auth/devstorage.read_write';

/**
* Retry strategy to signify that we never want to retry an operation
* even if the error is retryable.
*
* We can set $options['retryStrategy'] to one of "always", "never" and
* "idempotent".
*/
const RETRY_NEVER = 'never';
/**
* Retry strategy to signify that we always want to retry an operation.
*/
const RETRY_ALWAYS = 'always';
/**
* This is the default. This signifies that we want to retry an operation
* only if it is retryable and the error is retryable.
*/
const RETRY_IDEMPOTENT = 'idempotent';

/**
* @var ConnectionInterface Represents a connection to Storage.
*/
Expand Down
9 changes: 5 additions & 4 deletions Storage/tests/Unit/Connection/RestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use Google\Cloud\Core\Upload\StreamableUploader;
use Google\Cloud\Storage\Connection\Rest;
use Google\Cloud\Storage\Connection\RetryTrait;
use Google\Cloud\Storage\StorageClient;
use Google\CRC32\CRC32;
use GuzzleHttp\Promise;
use GuzzleHttp\Promise\PromiseInterface;
Expand Down Expand Up @@ -656,14 +657,14 @@ public function retryStrategyProvider()
return [
// The op is a conditionally idempotent operation,
// but it should still be retried because we pass the strategy as 'always'
[$retryAbleException, false, true, false, Rest::getStrategyAlwaysKey(), true],
[$retryAbleException, false, true, false, StorageClient::RETRY_ALWAYS, true],
// The op is an idempotent operation,
// but it should still not be retried because we pass the strategy as 'never'
[$retryAbleException, true, false, false, Rest::getStrategyNeverKey(), false],
[$retryAbleException, true, false, false, StorageClient::RETRY_NEVER, false],
// The op is a conditionally idempotent operation,
// so, the decision is based on the status of the precondition supplied by the user
[$retryAbleException, false, true, false, Rest::getStrategyIdempotentKey(), false],
[$retryAbleException, false, true, true, Rest::getStrategyIdempotentKey(), true],
[$retryAbleException, false, true, false, StorageClient::RETRY_IDEMPOTENT, false],
[$retryAbleException, false, true, true, StorageClient::RETRY_IDEMPOTENT, true],
];
}

Expand Down