cleanup(bigtable)!: remove experimental InstanceChannelAffinityOption (#16213)

Author: scotthart

CHANGELOG.md

@@ -18,7 +18,7 @@ the APIs in these libraries are stable, and are ready for production use.
 
 ### [Bigtable](/google/cloud/bigtable/README.md)
 
-- Explicit instance declaration is now encouraged during client initialization via a new overload of `MakeDataConnection` that takes a `std::vector<InstanceResource>`. Specifying the target instances at client startup enables optimizing connection pooling (pre-warming/priming channels) and telemetry.
+- Explicit instance declaration is now encouraged during client initialization via a new overload of `MakeDataConnection` that takes a `std::vector<InstanceResource>`. Specifying the target instances at client startup enables optimizing connection pooling (pre-warming/priming channels) and telemetry. The experimental `InstanceChannelAffinityOption` has been removed; the new `MakeDataConnection` overload should be used instead.
 
   ```cpp
   #include "google/cloud/bigtable/data_connection.h"

google/cloud/bigtable/data_connection.cc

@@ -177,7 +177,7 @@ bigtable::RowStream DataConnection::ExecuteQuery(bigtable::ExecuteQueryParams) {
 
 std::shared_ptr<DataConnection> MakeDataConnection(
     std::vector<InstanceResource> instances, Options options) {
-  options.set<experimental::InstanceChannelAffinityOption>(
+  options.set<bigtable_internal::InstanceChannelAffinityOption>(
       std::move(instances));
   return MakeDataConnection(std::move(options));
 }
@@ -196,7 +196,7 @@ std::shared_ptr<DataConnection> MakeDataConnection(Options options) {
       bigtable_internal::MakeMutateRowsLimiter(background->cq(), options);
   std::shared_ptr<DataConnection> conn;
 
-  if (options.has<experimental::InstanceChannelAffinityOption>()) {
+  if (options.has<bigtable_internal::InstanceChannelAffinityOption>()) {
     auto stub_creation_fn =
         [auth, cq = background->cq(), options](
             std::string_view instance_name,
@@ -206,7 +206,7 @@ std::shared_ptr<DataConnection> MakeDataConnection(Options options) {
         };
 
     auto affinity_stubs = bigtable_internal::CreateBigtableAffinityStubs(
-        options.get<experimental::InstanceChannelAffinityOption>(),
+        options.get<bigtable_internal::InstanceChannelAffinityOption>(),
         stub_creation_fn);
     conn = std::make_shared<bigtable_internal::DataConnectionImpl>(
         std::move(background),

google/cloud/bigtable/data_connection.h

@@ -175,8 +175,8 @@ class DataConnection {
  * pools to scale dynamically based on outstanding RPC load.
  *
  * The returned connection object should not be used directly; instead it
- * should be given to a `Table` instance, and methods should be invoked on
- * `Table`.
+ * should be given to a `Table` or `Client` object, and methods should be
+ * invoked there.
  *
  * The optional @p opts argument may be used to configure aspects of the
  * returned `DataConnection`. Expected options are any of the following options

google/cloud/bigtable/data_connection_test.cc

@@ -15,6 +15,7 @@
 #include "google/cloud/internal/disable_deprecation_warnings.inc"
 #include "google/cloud/bigtable/data_connection.h"
 #include "google/cloud/bigtable/internal/bigtable_stub_factory.h"
+#include "google/cloud/bigtable/internal/data_connection_impl.h"
 #include "google/cloud/bigtable/options.h"
 #include "google/cloud/common_options.h"
 #include "google/cloud/credentials.h"
@@ -73,24 +74,6 @@ TEST(MakeDataConnection, TracingEnabled) {
               Contains(SpanNamed("bigtable::Table::Apply")));
 }
 
-TEST(MakeDataConnection, InstanceChannelAffinityOption) {
-  InstanceResource instance_a{Project("my-project"), "instance-a"};
-  InstanceResource instance_b{Project("my-project"), "instance-b"};
-  auto conn =
-      MakeDataConnection(TestOptions()
-                             .set<AppProfileIdOption>("user-supplied")
-                             .set<experimental::InstanceChannelAffinityOption>(
-                                 {instance_a, instance_b}));
-  auto options = conn->options();
-  EXPECT_TRUE(options.has<DataBackoffPolicyOption>())
-      << "Options are not defaulted in MakeDataConnection()";
-  EXPECT_EQ(options.get<AppProfileIdOption>(), "user-supplied")
-      << "User supplied Options are overridden in MakeDataConnection()";
-  ASSERT_TRUE(options.has<experimental::InstanceChannelAffinityOption>());
-  EXPECT_THAT(options.get<experimental::InstanceChannelAffinityOption>().size(),
-              Eq(2));
-}
-
 TEST(MakeDataConnection, TracingDisabled) {
   auto span_catcher = testing_util::InstallSpanCatcher();
 
@@ -112,9 +95,10 @@ TEST(MakeDataConnection, WithInstances) {
       << "Options are not defaulted in MakeDataConnection()";
   EXPECT_EQ(options.get<AppProfileIdOption>(), "user-supplied")
       << "User supplied Options are overridden in MakeDataConnection()";
-  ASSERT_TRUE(options.has<experimental::InstanceChannelAffinityOption>());
-  EXPECT_THAT(options.get<experimental::InstanceChannelAffinityOption>().size(),
-              Eq(2));
+  ASSERT_TRUE(options.has<bigtable_internal::InstanceChannelAffinityOption>());
+  EXPECT_THAT(
+      options.get<bigtable_internal::InstanceChannelAffinityOption>().size(),
+      Eq(2));
 }
 
 }  // namespace

google/cloud/bigtable/internal/data_connection_impl.h

@@ -16,6 +16,7 @@
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_BIGTABLE_INTERNAL_DATA_CONNECTION_IMPL_H
 
 #include "google/cloud/bigtable/data_connection.h"
+#include "google/cloud/bigtable/instance_resource.h"
 #include "google/cloud/bigtable/internal/bigtable_stub.h"
 #include "google/cloud/bigtable/internal/mutate_rows_limiter.h"
 #include "google/cloud/bigtable/internal/operation_context_factory.h"
@@ -28,12 +29,19 @@
 #include "google/cloud/status_or.h"
 #include "google/cloud/version.h"
 #include <memory>
+#include <vector>
 
 namespace google {
 namespace cloud {
 namespace bigtable_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
+// TODO(#16216): Remove this option in favor of addind a member variable to
+// store the instances.
+struct InstanceChannelAffinityOption {
+  using Type = std::vector<bigtable::InstanceResource>;
+};
+
 bigtable::Row TransformReadModifyWriteRowResponse(
     google::bigtable::v2::ReadModifyWriteRowResponse response);
 

google/cloud/bigtable/options.h

@@ -177,24 +177,11 @@ struct QueryPlanRefreshFunctionRetryPolicyOption {
 };
 
 /**
- *  If set, a dynamic channel pool is created for each instance that requests
- *  are destined. Instances specified as part of this Option have dynamic
- *  channel pools created and primed as part of DataConnection construction. If
- *  no Instances are specified, then dynamic channel pool creation is deferred
- *  until the first request sent, increasing time to first byte latency.
- *
- * @note This option must be supplied to `MakeDataConnection()` in order to take
- * effect.
- */
-struct InstanceChannelAffinityOption {
-  using Type = std::vector<bigtable::InstanceResource>;
-};
-
-/**
- *  If the `InstanceChannelAffinityOption` is set, then all connections will be
- *  managed by a Dynamic Channel Pool. The `DynamicChannelPoolSizingPolicy` can
- *  be provided via the `DynamicChannelPoolSizingPolicyOption` and configures
- *  the behavior of the `DynamicChannelPool`.
+ * If `MakeDataConnection(std::vector<InstanceResource>, Options)` is called,
+ * then all connections will be managed by a Dynamic Channel Pool. The
+ * `DynamicChannelPoolSizingPolicy` can be provided via the
+ * `experimental::DynamicChannelPoolSizingPolicyOption` and configures the
+ * behavior of the `DynamicChannelPool`.
  */
 struct DynamicChannelPoolSizingPolicy {
   // Removing unused channels is not as performance critical as adding channels
@@ -313,9 +300,10 @@ struct MetricsPeriodOption {
 };
 
 using DataPolicyOptionList =
-    OptionList<DataRetryPolicyOption, DataBackoffPolicyOption,
+    OptionList<DataRetryPolicyOption, DataBackoffPolicyOption, DeadlineOption,
                IdempotentMutationPolicyOption, EnableMetricsOption,
-               MetricsPeriodOption>;
+               MetricsPeriodOption,
+               experimental::DynamicChannelPoolSizingPolicyOption>;
 
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace bigtable

google/cloud/bigtable/testing/table_integration_test.cc

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include "google/cloud/internal/disable_deprecation_warnings.inc"
 #include "google/cloud/bigtable/testing/table_integration_test.h"
 #include "google/cloud/bigtable/resource_names.h"
 #include "google/cloud/bigtable/testing/random_names.h"
@@ -126,11 +127,12 @@ void TableIntegrationTest::SetUp() {
   if (google::cloud::internal::GetEnv(
           "GOOGLE_CLOUD_CPP_BIGTABLE_TESTING_CHANNEL_POOL")
           .value_or("") == "dynamic") {
-    options.set<experimental::InstanceChannelAffinityOption>({});
+    data_connection_ = MakeDataConnection(
+        {InstanceResource(Project(project_id()), instance_id())},
+        std::move(options));
+  } else {
+    data_connection_ = MakeDataConnection(std::move(options));
   }
-  data_connection_ = MakeDataConnection(
-      {InstanceResource(Project(project_id()), instance_id())},
-      std::move(options));
 
   // In production, we cannot use `DropAllRows()` to cleanup the table because
   // the integration tests sometimes consume all the 'DropRowRangeGroup' quota.