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

feat: promote EnableServerRetriesOption #13698

Merged
merged 1 commit into from
Feb 29, 2024
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
3 changes: 1 addition & 2 deletions google/cloud/bigtable/internal/bulk_mutator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,7 @@ Status BulkMutator::MakeOneRequest(BigtableStub& stub,
auto context = std::make_shared<grpc::ClientContext>();
google::cloud::internal::ConfigureContext(*context, options);
retry_context_.PreCall(*context);
bool enable_server_retries =
options.get<internal::EnableServerRetriesOption>();
bool enable_server_retries = options.get<EnableServerRetriesOption>();

struct UnpackVariant {
BulkMutatorState& state;
Expand Down
1 change: 0 additions & 1 deletion google/cloud/bigtable/internal/bulk_mutator_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ using ::google::cloud::testing_util::chrono_literals::operator"" _ms; // NOLINT
using ::google::cloud::bigtable::testing::MockBigtableStub;
using ::google::cloud::bigtable::testing::MockMutateRowsLimiter;
using ::google::cloud::bigtable::testing::MockMutateRowsStream;
using ::google::cloud::internal::EnableServerRetriesOption;
using ::google::cloud::testing_util::StatusIs;
using ::testing::AnyOf;
using ::testing::Contains;
Expand Down
2 changes: 1 addition & 1 deletion google/cloud/bigtable/internal/data_connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ inline std::unique_ptr<bigtable::IdempotentMutationPolicy> idempotency_policy(
}

inline bool enable_server_retries(Options const& options) {
return options.get<internal::EnableServerRetriesOption>();
return options.get<EnableServerRetriesOption>();
}

} // namespace
Expand Down
12 changes: 6 additions & 6 deletions google/cloud/bigtable/internal/data_connection_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ TEST_F(DataConnectionTest, BulkApplyRetryInfoHeeded) {

auto conn = TestConnection(std::move(mock));
internal::OptionsSpan span(
CallOptions().set<internal::EnableServerRetriesOption>(true));
CallOptions().set<EnableServerRetriesOption>(true));
auto actual = conn->BulkApply(kTableName, std::move(mut));
CheckFailedMutations(actual, {});
}
Expand All @@ -810,7 +810,7 @@ TEST_F(DataConnectionTest, BulkApplyRetryInfoIgnored) {

auto conn = TestConnection(std::move(mock));
internal::OptionsSpan span(
CallOptions().set<internal::EnableServerRetriesOption>(false));
CallOptions().set<EnableServerRetriesOption>(false));
auto actual = conn->BulkApply(kTableName, std::move(mut));
CheckFailedMutations(actual, expected);
}
Expand Down Expand Up @@ -955,7 +955,7 @@ TEST_F(DataConnectionTest, ReadRowsRetryInfoHeeded) {

auto conn = TestConnection(std::move(mock));
internal::OptionsSpan span(
CallOptions().set<internal::EnableServerRetriesOption>(true));
CallOptions().set<EnableServerRetriesOption>(true));
auto reader = conn->ReadRowsFull(bigtable::ReadRowsParams{
kTableName, kAppProfile, TestRowSet(), 42, TestFilter(), true});
EXPECT_EQ(reader.begin(), reader.end());
Expand All @@ -975,7 +975,7 @@ TEST_F(DataConnectionTest, ReadRowsRetryInfoIgnored) {

auto conn = TestConnection(std::move(mock));
internal::OptionsSpan span(
CallOptions().set<internal::EnableServerRetriesOption>(false));
CallOptions().set<EnableServerRetriesOption>(false));
auto reader = conn->ReadRowsFull(bigtable::ReadRowsParams{
kTableName, kAppProfile, TestRowSet(), 42, TestFilter(), true});
std::vector<StatusOr<bigtable::Row>> rows;
Expand Down Expand Up @@ -1639,7 +1639,7 @@ TEST_F(DataConnectionTest, SampleRowsRetryInfoHeeded) {

auto conn = TestConnection(std::move(mock));
internal::OptionsSpan span(
CallOptions().set<internal::EnableServerRetriesOption>(true));
CallOptions().set<EnableServerRetriesOption>(true));
auto samples = conn->SampleRows(kTableName);
EXPECT_STATUS_OK(samples);
}
Expand All @@ -1657,7 +1657,7 @@ TEST_F(DataConnectionTest, SampleRowsRetryInfoIgnored) {

auto conn = TestConnection(std::move(mock));
internal::OptionsSpan span(
CallOptions().set<internal::EnableServerRetriesOption>(false));
CallOptions().set<EnableServerRetriesOption>(false));
auto samples = conn->SampleRows(kTableName);
EXPECT_THAT(samples, StatusIs(StatusCode::kPermissionDenied));
}
Expand Down
4 changes: 2 additions & 2 deletions google/cloud/bigtable/internal/defaults.cc
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,8 @@ Options DefaultDataOptions(Options opts) {
opts.set<bigtable::IdempotentMutationPolicyOption>(
bigtable::DefaultIdempotentMutationPolicy());
}
if (!opts.has<google::cloud::internal::EnableServerRetriesOption>()) {
opts.set<google::cloud::internal::EnableServerRetriesOption>(true);
if (!opts.has<EnableServerRetriesOption>()) {
opts.set<EnableServerRetriesOption>(true);
}
opts = DefaultOptions(std::move(opts));
return opts.set<EndpointOption>(opts.get<DataEndpointOption>());
Expand Down
3 changes: 1 addition & 2 deletions google/cloud/bigtable/internal/defaults_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#include "google/cloud/bigtable/internal/defaults.h"
#include "google/cloud/bigtable/internal/client_options_defaults.h"
#include "google/cloud/bigtable/options.h"
#include "google/cloud/connection_options.h"
#include "google/cloud/common_options.h"
#include "google/cloud/grpc_options.h"
#include "google/cloud/internal/background_threads_impl.h"
#include "google/cloud/status.h"
Expand Down Expand Up @@ -218,7 +218,6 @@ TEST(OptionsTest, DataAuthorityOption) {
}

TEST(OptionsTest, DataEnableServerRetriesOption) {
using ::google::cloud::internal::EnableServerRetriesOption;
auto options = DefaultDataOptions(Options{});
EXPECT_TRUE(options.get<EnableServerRetriesOption>());

Expand Down
18 changes: 18 additions & 0 deletions google/cloud/common_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,24 @@ struct ProxyOption {
using Type = ProxyConfig;
};

/**
* Let the server make retry decisions, when applicable.
*
* In some cases the server knows how to handle retry behavior better than the
* client. For example, if a server-side resource is exhausted and the server
* knows when it will come back online, it can tell the client exactly when to
* retry.
*
* If this option is enabled, any supplied retry, backoff, or idempotency
* policies may be overridden by a recommendation from the server.
*
* For example, the server may know it is safe to retry a non-idempotent
* request, or safe to retry a status code that is typically a permanent error.
*/
struct EnableServerRetriesOption {
using Type = bool;
};

/**
* A list of all the common options.
*/
Expand Down
18 changes: 0 additions & 18 deletions google/cloud/grpc_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,24 +229,6 @@ struct GrpcSetupPollOption {
using Type = std::function<void(grpc::ClientContext&)>;
};

/**
* Let the server make retry decisions, when applicable.
*
* In some cases the server knows how to handle retry behavior better than the
* client. For example, if a server-side resource is exhausted and the server
* knows when it will come back online, it can tell the client exactly when to
* retry.
*
* If this option is enabled, any supplied retry, backoff, or idempotency
* policies may be overridden by a recommendation from the server.
*
* For example, the server may know it is safe to retry a non-idempotent
* request, or safe to retry a status code that is typically a permanent error.
*/
struct EnableServerRetriesOption {
using Type = bool;
};

/// Configure the ClientContext using options.
void ConfigureContext(grpc::ClientContext& context, Options const& opts);

Expand Down