Skip to content

Commit 685424c

Browse files
feat(storage): Add integration tests for bidi operations and make ABORTED as retriable error (#15114)
* chore(ACv2): Add integration tests for fastbyte operations (#115) * feat: Make ABORTED as retriable error (#116) * test failure fix and address review comments * Improve some internal documentation * checkers-pr fix --------- Co-authored-by: Denis DelGrosso <[email protected]>
1 parent d3679b1 commit 685424c

17 files changed

+720
-67
lines changed

google/cloud/storage/async/client.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -378,9 +378,8 @@ class AsyncClient {
378378
incrementally until it is finalized. This means you can start an upload
379379
and append data to the object later.
380380
381-
You can finalize an appendable object in the first call itself by providing
382-
all the data in the initial upload. You can also explicitly Flush to ensure
383-
the data is persisted.
381+
You can either finalize the upload once all data is sent or close it to resume
382+
later.
384383
385384
The recovery can be done from most transient errors, including an unexpected
386385
closure of the streaming RPC used for the upload.
@@ -403,7 +402,7 @@ class AsyncClient {
403402
* @snippet{doc} async/client.h start-appendable-object-upload
404403
*
405404
* @param bucket_name the name of the bucket that contains the object.
406-
* @param object_name the name of the object to be read.
405+
* @param object_name the name of the object to be uploaded.
407406
* @param opts options controlling the behavior of this RPC, for example
408407
* the application may change the retry policy.
409408
*/
@@ -418,7 +417,7 @@ class AsyncClient {
418417
* @snippet{doc} async/client.h start-appendable-object-upload
419418
*
420419
* @param request the request contents, it must include the bucket name and
421-
* object names. Many other fields are optional.
420+
* object name. Many other fields are optional.
422421
* @param opts options controlling the behavior of this RPC, for example
423422
* the application may change the retry policy.
424423
*/

google/cloud/storage/async/connection.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ class AsyncConnection {
116116
virtual future<StatusOr<ReadPayload>> ReadObjectRange(ReadObjectParams p) = 0;
117117

118118
/**
119-
* A thin wrapper around the `WriteObject()` parameters for appendable object
119+
* A thin wrapper around the `StartAppendableObjectUpload()` parameters for
120+
* appendable object
120121
*/
121122
struct AppendableUploadParams {
122123
/// The bucket name and object name for the new object.
Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
// Copyright 2025 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_ASYNC_RETRY_POLICY_H
16+
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_ASYNC_RETRY_POLICY_H
17+
18+
#include "google/cloud/storage/version.h"
19+
#include "google/cloud/internal/backoff_policy.h"
20+
#include "google/cloud/internal/retry_policy_impl.h"
21+
#include "google/cloud/status.h"
22+
#include <memory>
23+
24+
namespace google {
25+
namespace cloud {
26+
namespace storage_experimental {
27+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
28+
namespace internal {
29+
/// Defines what error codes are permanent errors.
30+
struct StatusTraits {
31+
static bool IsPermanentFailure(Status const& status) {
32+
return status.code() != StatusCode::kDeadlineExceeded &&
33+
status.code() != StatusCode::kInternal &&
34+
status.code() != StatusCode::kResourceExhausted &&
35+
status.code() != StatusCode::kUnavailable &&
36+
status.code() != StatusCode::kAborted;
37+
}
38+
};
39+
} // namespace internal
40+
41+
/// The base class for the Storage library async retry policies.
42+
class AsyncRetryPolicy : public google::cloud::RetryPolicy {
43+
public:
44+
/// Creates a new instance of the policy, reset to the initial state.
45+
virtual std::unique_ptr<AsyncRetryPolicy> clone() const = 0;
46+
};
47+
48+
/**
49+
* A retry policy based on counting errors.
50+
*
51+
* This policy stops retrying if:
52+
* - An RPC returns a non-transient error.
53+
* - More than a prescribed number of transient failures is detected.
54+
*
55+
* In this class the following status codes are treated as transient errors:
56+
* - [`kDeadlineExceeded`](@ref google::cloud::StatusCode)
57+
* - [`kInternal`](@ref google::cloud::StatusCode)
58+
* - [`kResourceExhausted`](@ref google::cloud::StatusCode)
59+
* - [`kUnavailable`](@ref google::cloud::StatusCode)
60+
* - [`kAborted`](@ref google::cloud::StatusCode)
61+
*/
62+
class LimitedErrorCountRetryPolicy : public AsyncRetryPolicy {
63+
public:
64+
/**
65+
* Create an instance that tolerates up to @p maximum_failures transient
66+
* errors.
67+
*
68+
* @note Disable the retry loop by providing an instance of this policy with
69+
* @p maximum_failures == 0.
70+
*/
71+
explicit LimitedErrorCountRetryPolicy(int maximum_failures)
72+
: impl_(maximum_failures) {}
73+
74+
LimitedErrorCountRetryPolicy(LimitedErrorCountRetryPolicy&& rhs) noexcept
75+
: LimitedErrorCountRetryPolicy(rhs.maximum_failures()) {}
76+
LimitedErrorCountRetryPolicy(LimitedErrorCountRetryPolicy const& rhs) noexcept
77+
: LimitedErrorCountRetryPolicy(rhs.maximum_failures()) {}
78+
79+
int maximum_failures() const { return impl_.maximum_failures(); }
80+
81+
bool OnFailure(Status const& s) override { return impl_.OnFailure(s); }
82+
bool IsExhausted() const override { return impl_.IsExhausted(); }
83+
bool IsPermanentFailure(Status const& s) const override {
84+
return impl_.IsPermanentFailure(s);
85+
}
86+
std::unique_ptr<AsyncRetryPolicy> clone() const override {
87+
return std::make_unique<LimitedErrorCountRetryPolicy>(
88+
impl_.maximum_failures());
89+
}
90+
91+
// This is provided only for backwards compatibility.
92+
using BaseType = AsyncRetryPolicy;
93+
94+
private:
95+
google::cloud::internal::LimitedErrorCountRetryPolicy<internal::StatusTraits>
96+
impl_;
97+
};
98+
99+
/**
100+
* A retry policy based on elapsed time.
101+
*
102+
* This policy stops retrying if:
103+
* - An RPC returns a non-transient error.
104+
* - The elapsed time in the retry loop exceeds a prescribed duration.
105+
*
106+
* In this class the following status codes are treated as transient errors:
107+
* - [`kDeadlineExceeded`](@ref google::cloud::StatusCode)
108+
* - [`kInternal`](@ref google::cloud::StatusCode)
109+
* - [`kResourceExhausted`](@ref google::cloud::StatusCode)
110+
* - [`kUnavailable`](@ref google::cloud::StatusCode)
111+
* - [`kAborted`](@ref google::cloud::StatusCode)
112+
*/
113+
class LimitedTimeRetryPolicy : public AsyncRetryPolicy {
114+
public:
115+
/**
116+
* Constructor given a `std::chrono::duration<>` object.
117+
*
118+
* @tparam DurationRep a placeholder to match the `Rep` tparam for
119+
* @p maximum_duration's type. The semantics of this template parameter
120+
* are documented in `std::chrono::duration<>`. In brief, the underlying
121+
* arithmetic type used to store the number of ticks. For our purposes it
122+
* is simply a formal parameter.
123+
* @tparam DurationPeriod a placeholder to match the `Period` tparam for
124+
* @p maximum_duration's type. The semantics of this template parameter
125+
* are documented in `std::chrono::duration<>`. In brief, the length of
126+
* the tick in seconds, expressed as a `std::ratio<>`. For our purposes it
127+
* is simply a formal parameter.
128+
* @param maximum_duration the maximum time allowed before the policy expires,
129+
* while the application can express this time in any units they desire,
130+
* the class truncates to milliseconds.
131+
*
132+
* @see https://en.cppreference.com/w/cpp/chrono/duration for more details
133+
* about `std::chrono::duration`.
134+
*/
135+
template <typename DurationRep, typename DurationPeriod>
136+
explicit LimitedTimeRetryPolicy(
137+
std::chrono::duration<DurationRep, DurationPeriod> maximum_duration)
138+
: impl_(maximum_duration) {}
139+
140+
LimitedTimeRetryPolicy(LimitedTimeRetryPolicy&& rhs) noexcept
141+
: LimitedTimeRetryPolicy(rhs.maximum_duration()) {}
142+
LimitedTimeRetryPolicy(LimitedTimeRetryPolicy const& rhs) noexcept
143+
: LimitedTimeRetryPolicy(rhs.maximum_duration()) {}
144+
145+
std::chrono::milliseconds maximum_duration() const {
146+
return impl_.maximum_duration();
147+
}
148+
149+
bool OnFailure(Status const& s) override { return impl_.OnFailure(s); }
150+
bool IsExhausted() const override { return impl_.IsExhausted(); }
151+
bool IsPermanentFailure(Status const& s) const override {
152+
return impl_.IsPermanentFailure(s);
153+
}
154+
std::unique_ptr<AsyncRetryPolicy> clone() const override {
155+
return std::make_unique<LimitedTimeRetryPolicy>(impl_.maximum_duration());
156+
}
157+
158+
// This is provided only for backwards compatibility.
159+
using BaseType = RetryPolicy;
160+
161+
private:
162+
google::cloud::internal::LimitedTimeRetryPolicy<internal::StatusTraits> impl_;
163+
};
164+
165+
/// The backoff policy base class.
166+
using AsyncBackoffPolicy = ::google::cloud::internal::BackoffPolicy;
167+
168+
/// Implement truncated exponential backoff with randomization.
169+
using ExponentialAsyncBackoffPolicy =
170+
::google::cloud::internal::ExponentialBackoffPolicy;
171+
172+
/// Configure the resume policy used in a request, client, or connection.
173+
struct AsyncRetryPolicyOption {
174+
using Type = std::shared_ptr<AsyncRetryPolicy>;
175+
};
176+
177+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
178+
} // namespace storage_experimental
179+
} // namespace cloud
180+
} // namespace google
181+
182+
#endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_ASYNC_RETRY_POLICY_H
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
// Copyright 2025 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
#include "google/cloud/storage/async/retry_policy.h"
16+
#include <gmock/gmock.h>
17+
18+
namespace google {
19+
namespace cloud {
20+
namespace storage_experimental {
21+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
22+
namespace internal {
23+
namespace {
24+
25+
TEST(RetryPolicyTest, PermanentFailure) {
26+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
27+
Status(StatusCode::kCancelled, "cancelled")));
28+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
29+
Status(StatusCode::kUnknown, "unknown")));
30+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
31+
Status(StatusCode::kInvalidArgument, "invalid argument")));
32+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
33+
Status(StatusCode::kNotFound, "not found")));
34+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
35+
Status(StatusCode::kAlreadyExists, "already exists")));
36+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
37+
Status(StatusCode::kPermissionDenied, "permission denied")));
38+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
39+
Status(StatusCode::kFailedPrecondition, "failed precondition")));
40+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
41+
Status(StatusCode::kOutOfRange, "out of range")));
42+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
43+
Status(StatusCode::kUnimplemented, "unimplemented")));
44+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
45+
Status(StatusCode::kDataLoss, "data loss")));
46+
EXPECT_TRUE(StatusTraits::IsPermanentFailure(
47+
Status(StatusCode::kUnauthenticated, "unauthenticated")));
48+
49+
EXPECT_FALSE(StatusTraits::IsPermanentFailure(
50+
Status(StatusCode::kDeadlineExceeded, "deadline exceeded")));
51+
EXPECT_FALSE(StatusTraits::IsPermanentFailure(
52+
Status(StatusCode::kResourceExhausted, "resource exhausted")));
53+
EXPECT_FALSE(StatusTraits::IsPermanentFailure(
54+
Status(StatusCode::kAborted, "aborted")));
55+
EXPECT_FALSE(StatusTraits::IsPermanentFailure(
56+
Status(StatusCode::kInternal, "internal")));
57+
EXPECT_FALSE(StatusTraits::IsPermanentFailure(
58+
Status(StatusCode::kUnavailable, "unavailable")));
59+
}
60+
61+
} // namespace
62+
} // namespace internal
63+
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
64+
} // namespace storage_experimental
65+
} // namespace cloud
66+
} // namespace google

google/cloud/storage/google_cloud_cpp_storage_grpc.bzl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ google_cloud_cpp_storage_grpc_hdrs = [
2929
"async/reader.h",
3030
"async/reader_connection.h",
3131
"async/resume_policy.h",
32+
"async/retry_policy.h",
3233
"async/rewriter.h",
3334
"async/rewriter_connection.h",
3435
"async/token.h",

google/cloud/storage/google_cloud_cpp_storage_grpc.cmake

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ add_library(
8888
async/reader_connection.h
8989
async/resume_policy.cc
9090
async/resume_policy.h
91+
async/retry_policy.h
9192
async/rewriter.cc
9293
async/rewriter.h
9394
async/rewriter_connection.h
@@ -424,6 +425,7 @@ set(storage_client_grpc_unit_tests
424425
async/read_all_test.cc
425426
async/reader_test.cc
426427
async/resume_policy_test.cc
428+
async/retry_policy_test.cc
427429
async/rewriter_test.cc
428430
async/token_test.cc
429431
async/writer_test.cc

google/cloud/storage/internal/async/connection_impl.cc

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "google/cloud/storage/async/read_all.h"
1919
#include "google/cloud/storage/async/reader.h"
2020
#include "google/cloud/storage/async/resume_policy.h"
21+
#include "google/cloud/storage/async/retry_policy.h"
2122
#include "google/cloud/storage/internal/async/default_options.h"
2223
#include "google/cloud/storage/internal/async/handle_redirect_error.h"
2324
#include "google/cloud/storage/internal/async/insert_object.h"
@@ -63,9 +64,9 @@ namespace storage_internal {
6364
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
6465
namespace {
6566

66-
inline std::unique_ptr<storage::RetryPolicy> retry_policy(
67+
inline std::unique_ptr<storage_experimental::AsyncRetryPolicy> retry_policy(
6768
Options const& options) {
68-
return options.get<storage::RetryPolicyOption>()->clone();
69+
return options.get<storage_experimental::AsyncRetryPolicyOption>()->clone();
6970
}
7071

7172
inline std::unique_ptr<BackoffPolicy> backoff_policy(Options const& options) {
@@ -200,7 +201,8 @@ AsyncConnectionImpl::Open(OpenParams p) {
200201
auto resume_policy =
201202
current->get<storage_experimental::ResumePolicyOption>()();
202203

203-
auto retry = std::shared_ptr<storage::RetryPolicy>(retry_policy(*current));
204+
auto retry = std::shared_ptr<storage_experimental::AsyncRetryPolicy>(
205+
retry_policy(*current));
204206
auto backoff =
205207
std::shared_ptr<storage::BackoffPolicy>(backoff_policy(*current));
206208
auto const* function_name = __func__;
@@ -310,7 +312,8 @@ AsyncConnectionImpl::AppendableObjectUploadImpl(AppendableUploadParams p,
310312
std::int64_t persisted_size = 0;
311313
std::shared_ptr<storage::internal::HashFunction> hash_function =
312314
CreateHashFunction(*current);
313-
auto retry = std::shared_ptr<storage::RetryPolicy>(retry_policy(*current));
315+
auto retry = std::shared_ptr<storage_experimental::AsyncRetryPolicy>(
316+
retry_policy(*current));
314317
auto backoff =
315318
std::shared_ptr<storage::BackoffPolicy>(backoff_policy(*current));
316319
using StreamingRpcTimeout =

google/cloud/storage/internal/async/connection_impl_insert_test.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414

1515
#include "google/cloud/storage/async/idempotency_policy.h"
16+
#include "google/cloud/storage/async/retry_policy.h"
1617
#include "google/cloud/storage/internal/async/connection_impl.h"
1718
#include "google/cloud/storage/internal/async/default_options.h"
1819
#include "google/cloud/storage/testing/canonical_errors.h"
@@ -72,8 +73,8 @@ auto TestOptions(Options options = {}) {
7273
std::move(options),
7374
Options{}
7475
.set<GrpcNumChannelsOption>(1)
75-
.set<storage::RetryPolicyOption>(
76-
storage::LimitedErrorCountRetryPolicy(2).clone())
76+
.set<storage_experimental::AsyncRetryPolicyOption>(
77+
storage_experimental::LimitedErrorCountRetryPolicy(2).clone())
7778
.set<storage::BackoffPolicyOption>(
7879
storage::ExponentialBackoffPolicy(ms(1), ms(2), 2.0).clone()));
7980
return DefaultOptionsAsync(std::move(options));

0 commit comments

Comments
 (0)