From 75e4f692415a4cff085f43f1972077788bf526f4 Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Mon, 27 Apr 2026 15:15:40 -0400 Subject: [PATCH 1/2] fix(bigtable): treat NOT_FOUND and PERMISSION_DENIED on channel refresh as success --- .../cloud/bigtable/internal/bigtable_stub_factory.cc | 9 ++++++++- google/cloud/bigtable/internal/channel_usage.h | 10 +++++++++- google/cloud/bigtable/internal/channel_usage_test.cc | 12 +++++++++--- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/google/cloud/bigtable/internal/bigtable_stub_factory.cc b/google/cloud/bigtable/internal/bigtable_stub_factory.cc index c95d23a0db618..04b4bc1145a2e 100644 --- a/google/cloud/bigtable/internal/bigtable_stub_factory.cc +++ b/google/cloud/bigtable/internal/bigtable_stub_factory.cc @@ -117,7 +117,14 @@ std::shared_ptr CreateBigtableStubRandomTwoLeastUsed( self->set_last_refresh_status(s); } if (!s.ok()) { - GCP_LOG(WARNING) << "Failed to refresh connection. Error: " << s; + if (s.code() == StatusCode::kPermissionDenied || + s.code() == StatusCode::kNotFound) { + GCP_LOG(WARNING) + << "Connection refreshed; treating received Status as non-error: " + << s; + } else { + GCP_LOG(WARNING) << "Failed to refresh connection. Error: " << s; + } } }; diff --git a/google/cloud/bigtable/internal/channel_usage.h b/google/cloud/bigtable/internal/channel_usage.h index 2a2e6b583c4bc..b98498668743c 100644 --- a/google/cloud/bigtable/internal/channel_usage.h +++ b/google/cloud/bigtable/internal/channel_usage.h @@ -45,7 +45,7 @@ class ChannelUsage : public std::enable_shared_from_this> { StatusOr instant_outstanding_rpcs() { std::scoped_lock lk(mu_); - if (!last_refresh_status_.ok()) return last_refresh_status_; + if (!IsRefreshSuccessful(lk)) return last_refresh_status_; return outstanding_rpcs_; } @@ -78,6 +78,14 @@ class ChannelUsage : public std::enable_shared_from_this> { } private: + bool IsRefreshSuccessful(std::scoped_lock const&) const { + // kPermissionDenied and kNotFound are considered acceptable as those may + // result from a permissions configuration issue and not an actual failure. + return last_refresh_status_.code() == StatusCode::kOk || + last_refresh_status_.code() == StatusCode::kPermissionDenied || + last_refresh_status_.code() == StatusCode::kNotFound; + } + mutable std::mutex mu_; std::shared_ptr stub_; int outstanding_rpcs_ = 0; diff --git a/google/cloud/bigtable/internal/channel_usage_test.cc b/google/cloud/bigtable/internal/channel_usage_test.cc index baeccf1119198..4203377f3367f 100644 --- a/google/cloud/bigtable/internal/channel_usage_test.cc +++ b/google/cloud/bigtable/internal/channel_usage_test.cc @@ -63,12 +63,18 @@ TEST(ChannelUsageTest, InstantOutstandingRpcs) { TEST(ChannelUsageTest, SetLastRefreshStatus) { auto mock = std::make_shared(); auto channel = std::make_shared>(mock); - Status expected_status = internal::InternalError("uh oh"); + Status error_status = internal::InternalError("uh oh"); + Status not_found_status = internal::NotFoundError("not found"); + Status permission_denied_status = internal::PermissionDeniedError("denied"); (void)channel->AcquireStub(); EXPECT_THAT(channel->instant_outstanding_rpcs(), IsOkAndHolds(1)); - channel->set_last_refresh_status(expected_status); + channel->set_last_refresh_status(not_found_status); + EXPECT_THAT(channel->instant_outstanding_rpcs(), IsOkAndHolds(1)); + channel->set_last_refresh_status(permission_denied_status); + EXPECT_THAT(channel->instant_outstanding_rpcs(), IsOkAndHolds(1)); + channel->set_last_refresh_status(error_status); EXPECT_THAT(channel->instant_outstanding_rpcs(), - StatusIs(expected_status.code())); + StatusIs(error_status.code())); } TEST(ChannelUsageTest, MakeWeak) { From 3a1cc50e565cf277fa5444f5a585e59465681fef Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Mon, 27 Apr 2026 15:30:32 -0400 Subject: [PATCH 2/2] address review comments --- .../bigtable/internal/bigtable_stub_factory.cc | 3 +-- google/cloud/bigtable/internal/channel_usage.h | 13 ++++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/google/cloud/bigtable/internal/bigtable_stub_factory.cc b/google/cloud/bigtable/internal/bigtable_stub_factory.cc index 04b4bc1145a2e..fe294cfd05f07 100644 --- a/google/cloud/bigtable/internal/bigtable_stub_factory.cc +++ b/google/cloud/bigtable/internal/bigtable_stub_factory.cc @@ -117,8 +117,7 @@ std::shared_ptr CreateBigtableStubRandomTwoLeastUsed( self->set_last_refresh_status(s); } if (!s.ok()) { - if (s.code() == StatusCode::kPermissionDenied || - s.code() == StatusCode::kNotFound) { + if (ChannelUsage::IsSuccessfulRefreshStatus(s)) { GCP_LOG(WARNING) << "Connection refreshed; treating received Status as non-error: " << s; diff --git a/google/cloud/bigtable/internal/channel_usage.h b/google/cloud/bigtable/internal/channel_usage.h index b98498668743c..93d6ac1c098f1 100644 --- a/google/cloud/bigtable/internal/channel_usage.h +++ b/google/cloud/bigtable/internal/channel_usage.h @@ -43,6 +43,13 @@ class ChannelUsage : public std::enable_shared_from_this> { outstanding_rpcs_(initial_outstanding_rpcs), last_refresh_status_(std::move(last_refresh_status)) {} + static bool IsSuccessfulRefreshStatus(Status const& s) { + // kPermissionDenied and kNotFound are considered acceptable as those may + // result from a permissions configuration issue and not an actual failure. + return s.ok() || s.code() == StatusCode::kPermissionDenied || + s.code() == StatusCode::kNotFound; + } + StatusOr instant_outstanding_rpcs() { std::scoped_lock lk(mu_); if (!IsRefreshSuccessful(lk)) return last_refresh_status_; @@ -79,11 +86,7 @@ class ChannelUsage : public std::enable_shared_from_this> { private: bool IsRefreshSuccessful(std::scoped_lock const&) const { - // kPermissionDenied and kNotFound are considered acceptable as those may - // result from a permissions configuration issue and not an actual failure. - return last_refresh_status_.code() == StatusCode::kOk || - last_refresh_status_.code() == StatusCode::kPermissionDenied || - last_refresh_status_.code() == StatusCode::kNotFound; + return IsSuccessfulRefreshStatus(last_refresh_status_); } mutable std::mutex mu_;