From 900fc0e556fcf5b3b5fa6223a118f9f21ebacc34 Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Mon, 29 Jun 2026 14:07:41 -0400 Subject: [PATCH 1/3] cleanup(bigtable)!: remove experimental InstanceChannelAffinityOption --- CHANGELOG.md | 2 +- google/cloud/bigtable/data_connection.cc | 6 ++-- google/cloud/bigtable/data_connection.h | 4 +-- google/cloud/bigtable/data_connection_test.cc | 25 +++++++++-------- .../bigtable/internal/data_connection_impl.h | 6 ++++ google/cloud/bigtable/options.h | 28 ++++++------------- .../testing/table_integration_test.cc | 10 ++++--- 7 files changed, 40 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1838ef1c7d5b..dfe14f74693ba 100644 --- a/CHANGELOG.md +++ b/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`. 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`. 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" diff --git a/google/cloud/bigtable/data_connection.cc b/google/cloud/bigtable/data_connection.cc index f1edca85380d5..6fd19a08706ae 100644 --- a/google/cloud/bigtable/data_connection.cc +++ b/google/cloud/bigtable/data_connection.cc @@ -177,7 +177,7 @@ bigtable::RowStream DataConnection::ExecuteQuery(bigtable::ExecuteQueryParams) { std::shared_ptr MakeDataConnection( std::vector instances, Options options) { - options.set( + options.set( std::move(instances)); return MakeDataConnection(std::move(options)); } @@ -196,7 +196,7 @@ std::shared_ptr MakeDataConnection(Options options) { bigtable_internal::MakeMutateRowsLimiter(background->cq(), options); std::shared_ptr conn; - if (options.has()) { + if (options.has()) { auto stub_creation_fn = [auth, cq = background->cq(), options]( std::string_view instance_name, @@ -206,7 +206,7 @@ std::shared_ptr MakeDataConnection(Options options) { }; auto affinity_stubs = bigtable_internal::CreateBigtableAffinityStubs( - options.get(), + options.get(), stub_creation_fn); conn = std::make_shared( std::move(background), diff --git a/google/cloud/bigtable/data_connection.h b/google/cloud/bigtable/data_connection.h index a45810191bcf9..c3c2247cbd85a 100644 --- a/google/cloud/bigtable/data_connection.h +++ b/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 diff --git a/google/cloud/bigtable/data_connection_test.cc b/google/cloud/bigtable/data_connection_test.cc index 3a503d35f4c28..03057254b65ed 100644 --- a/google/cloud/bigtable/data_connection_test.cc +++ b/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" @@ -76,19 +77,20 @@ TEST(MakeDataConnection, TracingEnabled) { TEST(MakeDataConnection, InstanceChannelAffinityOption) { InstanceResource instance_a{Project("my-project"), "instance-a"}; InstanceResource instance_b{Project("my-project"), "instance-b"}; - auto conn = - MakeDataConnection(TestOptions() - .set("user-supplied") - .set( - {instance_a, instance_b})); + auto conn = MakeDataConnection( + TestOptions() + .set("user-supplied") + .set( + {instance_a, instance_b})); auto options = conn->options(); EXPECT_TRUE(options.has()) << "Options are not defaulted in MakeDataConnection()"; EXPECT_EQ(options.get(), "user-supplied") << "User supplied Options are overridden in MakeDataConnection()"; - ASSERT_TRUE(options.has()); - EXPECT_THAT(options.get().size(), - Eq(2)); + ASSERT_TRUE(options.has()); + EXPECT_THAT( + options.get().size(), + Eq(2)); } TEST(MakeDataConnection, TracingDisabled) { @@ -112,9 +114,10 @@ TEST(MakeDataConnection, WithInstances) { << "Options are not defaulted in MakeDataConnection()"; EXPECT_EQ(options.get(), "user-supplied") << "User supplied Options are overridden in MakeDataConnection()"; - ASSERT_TRUE(options.has()); - EXPECT_THAT(options.get().size(), - Eq(2)); + ASSERT_TRUE(options.has()); + EXPECT_THAT( + options.get().size(), + Eq(2)); } } // namespace diff --git a/google/cloud/bigtable/internal/data_connection_impl.h b/google/cloud/bigtable/internal/data_connection_impl.h index dffaeb365c993..754cd1750a075 100644 --- a/google/cloud/bigtable/internal/data_connection_impl.h +++ b/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,17 @@ #include "google/cloud/status_or.h" #include "google/cloud/version.h" #include +#include namespace google { namespace cloud { namespace bigtable_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +struct InstanceChannelAffinityOption { + using Type = std::vector; +}; + bigtable::Row TransformReadModifyWriteRowResponse( google::bigtable::v2::ReadModifyWriteRowResponse response); diff --git a/google/cloud/bigtable/options.h b/google/cloud/bigtable/options.h index 2fdcefa622598..62c8fe8e68c8f 100644 --- a/google/cloud/bigtable/options.h +++ b/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; -}; - -/** - * 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, Options)` is called, + * 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`. */ struct DynamicChannelPoolSizingPolicy { // Removing unused channels is not as performance critical as adding channels @@ -313,9 +300,10 @@ struct MetricsPeriodOption { }; using DataPolicyOptionList = - OptionList; + MetricsPeriodOption, + experimental::DynamicChannelPoolSizingPolicyOption>; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace bigtable diff --git a/google/cloud/bigtable/testing/table_integration_test.cc b/google/cloud/bigtable/testing/table_integration_test.cc index 64af56d6cf0b9..ab39945248a93 100644 --- a/google/cloud/bigtable/testing/table_integration_test.cc +++ b/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({}); + 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. From 14daf6033090c9537fea5c9d34e4b7c95c7e88ea Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Mon, 29 Jun 2026 14:16:10 -0400 Subject: [PATCH 2/3] address gemini review comments --- google/cloud/bigtable/data_connection_test.cc | 19 ------------------- google/cloud/bigtable/options.h | 4 ++-- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/google/cloud/bigtable/data_connection_test.cc b/google/cloud/bigtable/data_connection_test.cc index 03057254b65ed..e3df946eff21c 100644 --- a/google/cloud/bigtable/data_connection_test.cc +++ b/google/cloud/bigtable/data_connection_test.cc @@ -74,25 +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("user-supplied") - .set( - {instance_a, instance_b})); - auto options = conn->options(); - EXPECT_TRUE(options.has()) - << "Options are not defaulted in MakeDataConnection()"; - EXPECT_EQ(options.get(), "user-supplied") - << "User supplied Options are overridden in MakeDataConnection()"; - ASSERT_TRUE(options.has()); - EXPECT_THAT( - options.get().size(), - Eq(2)); -} - TEST(MakeDataConnection, TracingDisabled) { auto span_catcher = testing_util::InstallSpanCatcher(); diff --git a/google/cloud/bigtable/options.h b/google/cloud/bigtable/options.h index 62c8fe8e68c8f..92c9b010b1b0c 100644 --- a/google/cloud/bigtable/options.h +++ b/google/cloud/bigtable/options.h @@ -180,8 +180,8 @@ struct QueryPlanRefreshFunctionRetryPolicyOption { * If `MakeDataConnection(std::vector, Options)` is called, * 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`. + * `experimental::DynamicChannelPoolSizingPolicyOption` and configures the + * behavior of the `DynamicChannelPool`. */ struct DynamicChannelPoolSizingPolicy { // Removing unused channels is not as performance critical as adding channels From 239c565a063f31916dc6d96ed5edf6583dcca80c Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Mon, 29 Jun 2026 15:56:55 -0400 Subject: [PATCH 3/3] add TODO --- google/cloud/bigtable/internal/data_connection_impl.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/google/cloud/bigtable/internal/data_connection_impl.h b/google/cloud/bigtable/internal/data_connection_impl.h index 754cd1750a075..7722ae2755a0e 100644 --- a/google/cloud/bigtable/internal/data_connection_impl.h +++ b/google/cloud/bigtable/internal/data_connection_impl.h @@ -36,6 +36,8 @@ 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; };