diff --git a/CHANGELOG.md b/CHANGELOG.md index efb23dcc9219d..4753d676bef71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,12 @@ ## v1.35.0 - TBD +### [Common Libraries](https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/README.md) + +* Backoff policies are now cloned from their initial state, instead of their + current state. Any accumulated delay will be reset to its initial value in the + clone. The previous behavior was a bug, and thus it has been fixed. ([#7696](https://github.com/googleapis/google-cloud-cpp/pull/7696)) + ## v1.34.0 - 2021-12 ### [BigQuery](https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/bigquery/README.md) [IAM](https://github.com/googleapis/google-cloud-cpp/blob/main/google/cloud/iam/README.md) diff --git a/google/cloud/bigtable/rpc_backoff_policy.cc b/google/cloud/bigtable/rpc_backoff_policy.cc index 58b80430f2883..841623d0aeec6 100644 --- a/google/cloud/bigtable/rpc_backoff_policy.cc +++ b/google/cloud/bigtable/rpc_backoff_policy.cc @@ -30,7 +30,8 @@ ExponentialBackoffPolicy::ExponentialBackoffPolicy( } std::unique_ptr ExponentialBackoffPolicy::clone() const { - return std::unique_ptr(new ExponentialBackoffPolicy(*this)); + return std::unique_ptr( + new ExponentialBackoffPolicy(initial_delay_, maximum_delay_)); } void ExponentialBackoffPolicy::Setup(grpc::ClientContext&) const {} diff --git a/google/cloud/bigtable/rpc_backoff_policy.h b/google/cloud/bigtable/rpc_backoff_policy.h index 469e780df99ae..91f42cd35ee01 100644 --- a/google/cloud/bigtable/rpc_backoff_policy.h +++ b/google/cloud/bigtable/rpc_backoff_policy.h @@ -87,9 +87,14 @@ class ExponentialBackoffPolicy : public RPCBackoffPolicy { public: // NOLINTNEXTLINE(google-explicit-constructor) ExponentialBackoffPolicy(internal::RPCPolicyParameters defaults); - template - ExponentialBackoffPolicy(DurationT1 initial_delay, DurationT2 maximum_delay) - : impl_(initial_delay / 2, maximum_delay, 2.0) {} + template + ExponentialBackoffPolicy(std::chrono::duration initial_delay, + std::chrono::duration maximum_delay) + : initial_delay_(std::chrono::duration_cast( + initial_delay)), + maximum_delay_(std::chrono::duration_cast( + maximum_delay)), + impl_(initial_delay_ / 2, maximum_delay_, 2.0) {} std::unique_ptr clone() const override; void Setup(grpc::ClientContext& context) const override; @@ -99,6 +104,9 @@ class ExponentialBackoffPolicy : public RPCBackoffPolicy { std::chrono::milliseconds OnCompletion(grpc::Status const& status) override; private: + std::chrono::microseconds initial_delay_; + std::chrono::microseconds maximum_delay_; + using Impl = ::google::cloud::internal::ExponentialBackoffPolicy; Impl impl_; }; diff --git a/google/cloud/bigtable/rpc_backoff_policy_test.cc b/google/cloud/bigtable/rpc_backoff_policy_test.cc index 57db71e8a9cf8..d423aec2568a6 100644 --- a/google/cloud/bigtable/rpc_backoff_policy_test.cc +++ b/google/cloud/bigtable/rpc_backoff_policy_test.cc @@ -52,6 +52,10 @@ TEST(ExponentialBackoffRetryPolicy, Clone) { EXPECT_GE(10_ms, tested->OnCompletion(CreateTransientError())); EXPECT_LE(10_ms, tested->OnCompletion(CreateTransientError())); + + // Ensure the initial state of the policy is cloned, not the current state. + tested = tested->clone(); + EXPECT_GE(10_ms, tested->OnCompletion(CreateTransientError())); } /// @test Test for testing randomness for 2 objects of diff --git a/google/cloud/internal/backoff_policy.cc b/google/cloud/internal/backoff_policy.cc index 4b6f3a5492962..d2898487e9b10 100644 --- a/google/cloud/internal/backoff_policy.cc +++ b/google/cloud/internal/backoff_policy.cc @@ -21,7 +21,8 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN namespace internal { std::unique_ptr ExponentialBackoffPolicy::clone() const { - return absl::make_unique(*this); + return absl::make_unique(initial_delay_, + maximum_delay_, scaling_); } std::chrono::milliseconds ExponentialBackoffPolicy::OnCompletion() { diff --git a/google/cloud/internal/backoff_policy.h b/google/cloud/internal/backoff_policy.h index ddf78817ce72c..44098392b5c14 100644 --- a/google/cloud/internal/backoff_policy.h +++ b/google/cloud/internal/backoff_policy.h @@ -126,9 +126,9 @@ class ExponentialBackoffPolicy : public BackoffPolicy { ExponentialBackoffPolicy(std::chrono::duration initial_delay, std::chrono::duration maximum_delay, double scaling) - : current_delay_range_( - std::chrono::duration_cast( - 2 * initial_delay)), + : initial_delay_(std::chrono::duration_cast( + initial_delay)), + current_delay_range_(2 * initial_delay_), maximum_delay_(std::chrono::duration_cast( maximum_delay)), scaling_(scaling) { @@ -143,7 +143,8 @@ class ExponentialBackoffPolicy : public BackoffPolicy { // know specifically which one is at fault) // - We want uncorrelated data streams for each copy anyway. ExponentialBackoffPolicy(ExponentialBackoffPolicy const& rhs) noexcept - : current_delay_range_(rhs.current_delay_range_), + : initial_delay_(rhs.initial_delay_), + current_delay_range_(rhs.current_delay_range_), maximum_delay_(rhs.maximum_delay_), scaling_(rhs.scaling_) {} @@ -151,6 +152,7 @@ class ExponentialBackoffPolicy : public BackoffPolicy { std::chrono::milliseconds OnCompletion() override; private: + std::chrono::microseconds initial_delay_; std::chrono::microseconds current_delay_range_; std::chrono::microseconds maximum_delay_; double scaling_; diff --git a/google/cloud/internal/backoff_policy_test.cc b/google/cloud/internal/backoff_policy_test.cc index 7b7658b37c580..38c550a48a616 100644 --- a/google/cloud/internal/backoff_policy_test.cc +++ b/google/cloud/internal/backoff_policy_test.cc @@ -88,6 +88,12 @@ TEST(ExponentialBackoffPolicy, Clone) { delay = tested->OnCompletion(); EXPECT_LE(ms(25), delay); EXPECT_GE(ms(50), delay); + + // Ensure the initial state of the policy is cloned, not the current state. + tested = tested->clone(); + delay = tested->OnCompletion(); + EXPECT_LE(ms(10), delay); + EXPECT_GE(ms(20), delay); } /// @test Test for testing randomness for 2 objects of