From 0b1549b3f596c19e2d9a32da6634b1ce9f1a8b5c Mon Sep 17 00:00:00 2001 From: Anna Levenberg Date: Mon, 13 May 2024 14:01:24 -0400 Subject: [PATCH] cleanup(bigtable): use `Status` factory functions (#14223) * cleanup(bigtable): use `Status` factory functions * checkers and fix * fix * fix clang and generate libraries --- google/cloud/bigtable/benchmarks/benchmark.cc | 5 +- .../mutation_batcher_throughput_benchmark.cc | 5 +- .../mutation_batcher_throughput_options.cc | 4 +- google/cloud/bigtable/client_options.h | 8 +- google/cloud/bigtable/instance_resource.cc | 6 +- .../cloud/bigtable/internal/bulk_mutator.cc | 13 +-- .../bigtable/internal/data_connection_impl.cc | 8 +- google/cloud/bigtable/table.cc | 99 +++++++++++-------- google/cloud/bigtable/table.h | 9 +- google/cloud/bigtable/table_resource.cc | 5 +- .../bigtable/test_proxy/cbt_test_proxy.cc | 5 +- 11 files changed, 96 insertions(+), 71 deletions(-) diff --git a/google/cloud/bigtable/benchmarks/benchmark.cc b/google/cloud/bigtable/benchmarks/benchmark.cc index cdb040807402..4cf7879db193 100644 --- a/google/cloud/bigtable/benchmarks/benchmark.cc +++ b/google/cloud/bigtable/benchmarks/benchmark.cc @@ -18,6 +18,7 @@ #include "google/cloud/bigtable/resource_names.h" #include "google/cloud/internal/background_threads_impl.h" #include "google/cloud/internal/getenv.h" +#include "google/cloud/internal/make_status.h" #include #include #include @@ -44,8 +45,8 @@ google::cloud::StatusOr ParseArgs( if (!value.empty()) continue; std::ostringstream os; os << "The environment variable " << var << " is not set or empty"; - return google::cloud::Status(google::cloud::StatusCode::kUnknown, - std::move(os).str()); + return google::cloud::internal::UnknownError(std::move(os).str(), + GCP_ERROR_INFO()); } auto const project_id = GetEnv("GOOGLE_CLOUD_PROJECT").value(); auto const instance_id = diff --git a/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_benchmark.cc b/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_benchmark.cc index 77c5423ba5c0..d02b8f1c90dd 100644 --- a/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_benchmark.cc +++ b/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_benchmark.cc @@ -20,6 +20,7 @@ #include "google/cloud/bigtable/testing/random_names.h" #include "google/cloud/internal/background_threads_impl.h" #include "google/cloud/internal/getenv.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/status_or.h" #include "google/cloud/testing_util/command_line_parsing.h" #include "absl/time/time.h" @@ -36,7 +37,6 @@ namespace cbt = ::google::cloud::bigtable; using cbt::benchmarks::MutationBatcherThroughputOptions; using cbt::benchmarks::ParseMutationBatcherThroughputOptions; using ::google::cloud::Status; -using ::google::cloud::StatusCode; using ::google::cloud::StatusOr; using ::google::cloud::internal::GetEnv; @@ -93,7 +93,8 @@ StatusOr ParseArgs(int argc, char* argv[]) { if (!value.empty()) continue; std::ostringstream os; os << "The environment variable " << var << "is not set or empty"; - return Status(StatusCode::kUnknown, std::move(os).str()); + return google::cloud::internal::UnknownError(std::move(os).str(), + GCP_ERROR_INFO()); } auto const project_id = GetEnv("GOOGLE_CLOUD_PROJECT").value(); auto const instance_id = diff --git a/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_options.cc b/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_options.cc index fc0af4407c15..5734b3afba3e 100644 --- a/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_options.cc +++ b/google/cloud/bigtable/benchmarks/mutation_batcher_throughput_options.cc @@ -14,6 +14,7 @@ #include "google/cloud/bigtable/benchmarks/mutation_batcher_throughput_options.h" #include "google/cloud/internal/absl_str_join_quiet.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/status_or.h" #include "google/cloud/testing_util/command_line_parsing.h" #include @@ -103,8 +104,7 @@ ParseMutationBatcherThroughputOptions(std::vector const& argv, } auto make_status = [](std::ostringstream& os) { - auto const code = google::cloud::StatusCode::kInvalidArgument; - return google::cloud::Status{code, std::move(os).str()}; + return internal::InvalidArgumentError(std::move(os).str()); }; if (unparsed.size() != 1) { diff --git a/google/cloud/bigtable/client_options.h b/google/cloud/bigtable/client_options.h index b4587af0ad37..d50e981e81a6 100644 --- a/google/cloud/bigtable/client_options.h +++ b/google/cloud/bigtable/client_options.h @@ -22,6 +22,7 @@ #include "google/cloud/completion_queue.h" #include "google/cloud/grpc_options.h" #include "google/cloud/internal/algorithm.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/options.h" #include "google/cloud/status.h" #include "google/cloud/tracing_options.h" @@ -365,9 +366,10 @@ class ClientOptions { std::chrono::duration_cast(fallback_timeout); if (ft_ms.count() > std::numeric_limits::max()) { - return google::cloud::Status(google::cloud::StatusCode::kOutOfRange, - "The supplied duration is larger than the " - "maximum value allowed by gRPC (INT_MAX)"); + return google::cloud::internal::OutOfRangeError( + "The supplied duration is larger than the " + "maximum value allowed by gRPC (INT_MAX)", + GCP_ERROR_INFO()); } auto fallback_timeout_ms = static_cast(ft_ms.count()); opts_.lookup().SetGrpclbFallbackTimeout( diff --git a/google/cloud/bigtable/instance_resource.cc b/google/cloud/bigtable/instance_resource.cc index a55c17b349b9..a6cf8b6e1156 100644 --- a/google/cloud/bigtable/instance_resource.cc +++ b/google/cloud/bigtable/instance_resource.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/bigtable/instance_resource.h" +#include "google/cloud/internal/make_status.h" #include #include @@ -44,8 +45,9 @@ StatusOr MakeInstanceResource(std::string const& full_name) { std::regex re("projects/([^/]+)/instances/([^/]+)"); std::smatch matches; if (!std::regex_match(full_name, matches, re)) { - return Status(StatusCode::kInvalidArgument, - "Improperly formatted InstanceResource: " + full_name); + return internal::InvalidArgumentError( + "Improperly formatted InstanceResource: " + full_name, + GCP_ERROR_INFO()); } return InstanceResource(Project(std::move(matches[1])), std::move(matches[2])); diff --git a/google/cloud/bigtable/internal/bulk_mutator.cc b/google/cloud/bigtable/internal/bulk_mutator.cc index 6db256403797..58d9baa140ac 100644 --- a/google/cloud/bigtable/internal/bulk_mutator.cc +++ b/google/cloud/bigtable/internal/bulk_mutator.cc @@ -15,6 +15,7 @@ #include "google/cloud/bigtable/internal/bulk_mutator.h" #include "google/cloud/bigtable/rpc_retry_policy.h" #include "google/cloud/bigtable/table.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/log.h" #include @@ -146,12 +147,12 @@ void BulkMutatorState::OnFinish(Status finish_status, pending_annotations_.push_back(std::move(annotation)); } else { if (last_status_.ok()) { - Status status( - StatusCode::kInternal, + auto status = internal::InternalError( "The server never sent a confirmation for this mutation but the " "stream didn't fail either. This is most likely a bug, please " "report it at " - "https://github.com/googleapis/google-cloud-cpp/issues/new"); + "https://github.com/googleapis/google-cloud-cpp/issues/new", + GCP_ERROR_INFO()); failures_.emplace_back(std::move(status), annotation.original_index); } else { failures_.emplace_back(last_status_, annotation.original_index); @@ -173,12 +174,12 @@ std::vector BulkMutatorState::OnRetryDone() && { } else if (!last_status_.ok()) { result.emplace_back(last_status_, annotation.original_index); } else { - Status status( - StatusCode::kInternal, + auto status = internal::InternalError( "The server never sent a confirmation for this mutation but the " "stream didn't fail either. This is most likely a bug, please " "report it at " - "https://github.com/googleapis/google-cloud-cpp/issues/new"); + "https://github.com/googleapis/google-cloud-cpp/issues/new", + GCP_ERROR_INFO()); result.emplace_back(std::move(status), annotation.original_index); } } diff --git a/google/cloud/bigtable/internal/data_connection_impl.cc b/google/cloud/bigtable/internal/data_connection_impl.cc index f81d35e8f751..41e7675677d2 100644 --- a/google/cloud/bigtable/internal/data_connection_impl.cc +++ b/google/cloud/bigtable/internal/data_connection_impl.cc @@ -25,6 +25,7 @@ #include "google/cloud/grpc_options.h" #include "google/cloud/idempotency.h" #include "google/cloud/internal/async_retry_loop.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/internal/retry_loop.h" #include #include @@ -227,9 +228,10 @@ StatusOr> DataConnectionImpl::ReadRow( if (!*it) return it->status(); auto result = std::make_pair(true, std::move(**it)); if (++it != reader.end()) { - return Status( - StatusCode::kInternal, - "internal error - RowReader returned more than one row in ReadRow()"); + return internal::InternalError( + "internal error - RowReader returned more than one row in ReadRow(, " + ")", + GCP_ERROR_INFO()); } return result; } diff --git a/google/cloud/bigtable/table.cc b/google/cloud/bigtable/table.cc index 7ec3cce98805..6ff58d338b75 100644 --- a/google/cloud/bigtable/table.cc +++ b/google/cloud/bigtable/table.cc @@ -20,6 +20,7 @@ #include "google/cloud/bigtable/internal/legacy_row_reader.h" #include "google/cloud/bigtable/internal/unary_client_utils.h" #include "google/cloud/internal/async_retry_unary_rpc.h" +#include "google/cloud/internal/make_status.h" #include #include @@ -54,9 +55,10 @@ Status Table::Apply(SingleRowMutation mut, Options opts) { return connection_->Apply(table_name_, std::move(mut)); } if (!google::cloud::internal::IsEmpty(opts)) { - return Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."); + return google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()); } // Copy the policies in effect for this operation. Many policy classes change @@ -106,10 +108,10 @@ future Table::AsyncApply(SingleRowMutation mut, Options opts) { return connection_->AsyncApply(table_name_, std::move(mut)); } if (!google::cloud::internal::IsEmpty(opts)) { - return make_ready_future( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`.")); + return make_ready_future(google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO())); } google::bigtable::v2::MutateRowRequest request; @@ -156,9 +158,10 @@ std::vector Table::BulkApply(BulkMutation mut, Options opts) { } if (!google::cloud::internal::IsEmpty(opts)) { return bigtable_internal::MakeFailedMutations( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."), + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()), mut.size()); } @@ -196,9 +199,10 @@ future> Table::AsyncBulkApply(BulkMutation mut, } if (!google::cloud::internal::IsEmpty(opts)) { return make_ready_future(bigtable_internal::MakeFailedMutations( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."), + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()), mut.size())); } @@ -225,9 +229,10 @@ RowReader Table::ReadRows(RowSet row_set, std::int64_t rows_limit, if (!google::cloud::internal::IsEmpty(opts)) { return MakeRowReader( std::make_shared( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."))); + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()))); } auto impl = std::make_shared( @@ -246,9 +251,10 @@ StatusOr> Table::ReadRow(std::string row_key, std::move(filter)); } if (!google::cloud::internal::IsEmpty(opts)) { - return Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."); + return google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()); } RowSet row_set(std::move(row_key)); @@ -265,9 +271,9 @@ StatusOr> Table::ReadRow(std::string row_key, } auto result = std::make_pair(true, std::move(**it)); if (++it != reader.end()) { - return Status( - StatusCode::kInternal, - "internal error - RowReader returned more than one row in ReadRow()"); + return google::cloud::internal::InternalError( + "internal error - RowReader returned more than one row in ReadRow(, " + "GCP_ERROR_INFO())"); } return result; } @@ -282,9 +288,10 @@ StatusOr Table::CheckAndMutateRow( std::move(true_mutations), std::move(false_mutations)); } if (!google::cloud::internal::IsEmpty(opts)) { - return Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."); + return google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()); } grpc::Status status; @@ -325,9 +332,10 @@ future> Table::AsyncCheckAndMutateRow( } if (!google::cloud::internal::IsEmpty(opts)) { return make_ready_future>( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`.")); + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO())); } btproto::CheckAndMutateRowRequest request; @@ -382,9 +390,10 @@ StatusOr> Table::SampleRows(Options opts) { return connection_->SampleRows(table_name_); } if (!google::cloud::internal::IsEmpty(opts)) { - return Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."); + return google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()); } // Copy the policies in effect for this operation. @@ -435,9 +444,10 @@ future>> Table::AsyncSampleRows( } if (!google::cloud::internal::IsEmpty(opts)) { return make_ready_future>>( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`.")); + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO())); } auto cq = background_threads_->cq(); @@ -456,9 +466,10 @@ StatusOr Table::ReadModifyWriteRowImpl( return connection_->ReadModifyWriteRow(std::move(request)); } if (!google::cloud::internal::IsEmpty(opts)) { - return Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`."); + return google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO()); } grpc::Status status; @@ -484,9 +495,10 @@ future> Table::AsyncReadModifyWriteRowImpl( } if (!google::cloud::internal::IsEmpty(opts)) { return make_ready_future>( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`.")); + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO())); } auto cq = background_threads_->cq(); @@ -522,9 +534,10 @@ future>> Table::AsyncReadRow(std::string row_key, } if (!google::cloud::internal::IsEmpty(opts)) { return make_ready_future>>( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`.")); + google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO())); } class AsyncReadRowHandler { diff --git a/google/cloud/bigtable/table.h b/google/cloud/bigtable/table.h index 75d8d5b55c3a..00b16e69098a 100644 --- a/google/cloud/bigtable/table.h +++ b/google/cloud/bigtable/table.h @@ -37,6 +37,7 @@ #include "google/cloud/future.h" #include "google/cloud/grpc_error_delegate.h" #include "google/cloud/internal/group_options.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/options.h" #include "google/cloud/status.h" #include "google/cloud/status_or.h" @@ -783,10 +784,10 @@ class Table { return; } if (!google::cloud::internal::IsEmpty(opts)) { - on_finish_fn( - Status(StatusCode::kInvalidArgument, - "Per-operation options only apply to `Table`s constructed " - "with a `DataConnection`.")); + on_finish_fn(google::cloud::internal::InvalidArgumentError( + "Per-operation options only apply to `Table`s constructed " + "with a `DataConnection`.", + GCP_ERROR_INFO())); return; } diff --git a/google/cloud/bigtable/table_resource.cc b/google/cloud/bigtable/table_resource.cc index f75671b3f53b..c1f10760bb6d 100644 --- a/google/cloud/bigtable/table_resource.cc +++ b/google/cloud/bigtable/table_resource.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/bigtable/table_resource.h" +#include "google/cloud/internal/make_status.h" #include #include @@ -50,8 +51,8 @@ StatusOr MakeTableResource(std::string const& full_name) { std::regex re("projects/([^/]+)/instances/([^/]+)/tables/([^/]+)"); std::smatch matches; if (!std::regex_match(full_name, matches, re)) { - return Status(StatusCode::kInvalidArgument, - "Improperly formatted TableResource: " + full_name); + return internal::InvalidArgumentError( + "Improperly formatted TableResource: " + full_name, GCP_ERROR_INFO()); } return TableResource( InstanceResource(Project(std::move(matches[1])), std::move(matches[2])), diff --git a/google/cloud/bigtable/test_proxy/cbt_test_proxy.cc b/google/cloud/bigtable/test_proxy/cbt_test_proxy.cc index 21d8095aeb26..5eaf14cad861 100644 --- a/google/cloud/bigtable/test_proxy/cbt_test_proxy.cc +++ b/google/cloud/bigtable/test_proxy/cbt_test_proxy.cc @@ -18,6 +18,7 @@ #include "google/cloud/bigtable/mutations.h" #include "google/cloud/bigtable/table.h" #include "google/cloud/internal/absl_str_cat_quiet.h" +#include "google/cloud/internal/make_status.h" #include "google/cloud/log.h" #include "google/cloud/status.h" #include "absl/time/time.h" @@ -364,8 +365,8 @@ StatusOr> CbtTestProxy::GetConnection( std::lock_guard lk(mu_); auto client_it = connections_.find(client_id); if (client_it == connections_.end()) { - return Status(StatusCode::kNotFound, - absl::StrCat("Client ", client_id, " not found.")); + return google::cloud::internal::NotFoundError( + absl::StrCat("Client ", client_id, " not found."), GCP_ERROR_INFO()); } return client_it->second;