Skip to content

cleanup(bigtable)!: remove experimental InstanceChannelAffinityOption #16213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
scotthart merged 3 commits into from
Jun 29, 2026
Merged

cleanup(bigtable)!: remove experimental InstanceChannelAffinityOption #16213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
900fc0e
cleanup(bigtable)!: remove experimental InstanceChannelAffinityOption
scotthart Jun 29, 2026
14daf60
address gemini review comments
scotthart Jun 29, 2026
239c565
add TODO
scotthart Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
6 changes: 3 additions & 3 deletions google/cloud/bigtable/data_connection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -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,
Expand All @@ -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),
Expand Down
4 changes: 2 additions & 2 deletions google/cloud/bigtable/data_connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
dbolduc marked this conversation as resolved.
*
* The optional @p opts argument may be used to configure aspects of the
* returned `DataConnection`. Expected options are any of the following options
Expand Down
26 changes: 5 additions & 21 deletions google/cloud/bigtable/data_connection_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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();

Expand All @@ -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
Expand Down
8 changes: 8 additions & 0 deletions google/cloud/bigtable/internal/data_connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {

@dbolduc dbolduc Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: internal options are a code smell. I am guessing we moved the option to internal to keep the refactor small.

In the long run, it seems ideal for the concrete DataConnectionImpl to hold these std::vector<InstanceResources> as a member.

@scotthart scotthart Jun 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Created #16216 to track.

using Type = std::vector<bigtable::InstanceResource>;
};

bigtable::Row TransformReadModifyWriteRowResponse(
google::bigtable::v2::ReadModifyWriteRowResponse response);

Expand Down
28 changes: 8 additions & 20 deletions google/cloud/bigtable/options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions google/cloud/bigtable/testing/table_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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));

@dbolduc dbolduc Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this deprecated? Also, options is empty.

Maybe like:

auto instances = std::vector<InstanceResource>{};
if (GetEnv("GOOGLE_CLOUD_CPP_BIGTABLE_TESTING_CHANNEL_POOL")
          .value_or("") == "dynamic") {
  instances.push_back(InstanceResource(Project(project_id()), instance_id()));
}
data_connection_ = MakeDataConnection(std::move(instances));

Q: is there a downside to always supplying the InstanceResource, even when not using the dynamic channel pool? Then it is just:

    data_connection_ = MakeDataConnection(
        {InstanceResource(Project(project_id()), instance_id())});

@scotthart scotthart Jun 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the deprecated overload results in a single, static, round-robin channel pool. Using the new overload always uses DynamicChannelPools.

Using the deprecated method allows continued testing of the old channel pool.

}
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.
Expand Down
Loading