fix(bigtable): treat NOT_FOUND and PERMISSION_DENIED on channel refresh as success (#16086)

Author: scotthart

google/cloud/bigtable/internal/bigtable_stub_factory.cc

@@ -117,7 +117,13 @@ std::shared_ptr<BigtableStub> CreateBigtableStubRandomTwoLeastUsed(
         self->set_last_refresh_status(s);
       }
       if (!s.ok()) {
-        GCP_LOG(WARNING) << "Failed to refresh connection. Error: " << s;
+        if (ChannelUsage<BigtableStub>::IsSuccessfulRefreshStatus(s)) {
+          GCP_LOG(WARNING)
+              << "Connection refreshed; treating received Status as non-error: "
+              << s;
+        } else {
+          GCP_LOG(WARNING) << "Failed to refresh connection. Error: " << s;
+        }
       }
     };
 

google/cloud/bigtable/internal/channel_usage.h

@@ -43,9 +43,16 @@ class ChannelUsage : public std::enable_shared_from_this<ChannelUsage<T>> {
         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<int> 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 +85,10 @@ class ChannelUsage : public std::enable_shared_from_this<ChannelUsage<T>> {
   }
 
  private:
+  bool IsRefreshSuccessful(std::scoped_lock<std::mutex> const&) const {
+    return IsSuccessfulRefreshStatus(last_refresh_status_);
+  }
+
   mutable std::mutex mu_;
   std::shared_ptr<T> stub_;
   int outstanding_rpcs_ = 0;

google/cloud/bigtable/internal/channel_usage_test.cc

@@ -63,12 +63,18 @@ TEST(ChannelUsageTest, InstantOutstandingRpcs) {
 TEST(ChannelUsageTest, SetLastRefreshStatus) {
   auto mock = std::make_shared<MockBigtableStub>();
   auto channel = std::make_shared<ChannelUsage<BigtableStub>>(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) {