feat(bigtable): add per-method session diversion configuration by mutianf · Pull Request #14681 · googleapis/google-cloud-java

mutianf · 2026-10-18T02:00:35Z

Summary

Adds a methodScopeKey to DivertingUnaryCallable so session load can be configured per-RPC-method via SessionDiversionConfiguration, in addition to the existing global session_load.
resolveSessionLoad() checks global_diversion or per_scope_diversion depending on which strategy the server returns; falls back to flat session_load when no diversion config is present.
Fixes ClientConfigurationManager to preserve session config when a SessionDiversionConfiguration is present even if the global session_load is 0 (diversion config encodes its own per-scope session loads).

Test plan

DivertingUnaryCallableTest: covers global diversion (load=1 routes to experimental, load=0 falls back to classic), per-scope match (routes to experimental), and per-scope non-match (falls back to classic)
Existing session routing tests continue to pass unchanged

gemini-code-assist

Code Review

This pull request refactors the Bigtable client's session diversion and shim layers, introducing support for global and per-scope session diversion configurations in DivertingUnaryCallable and refactoring ReadRowShim and ReadModifyWriteRowShim into stateless wrappers around inner stateful shims. However, the refactoring in ShimImpl.java introduced several compilation errors. Specifically, ReadRowShimInner is not imported, the field declarations do not match their initializations in the constructor, and decorateReadRow incorrectly attempts to use the removed readRowShim field instead of instantiating the new generic ReadRowShim class.

Add a methodScopeKey to DivertingUnaryCallable so session load can be configured per-RPC-method via SessionDiversionConfiguration, in addition to the existing global session_load. Resolve load from global_diversion or per_scope_diversion based on which strategy the server returns. Also move the supports() check before resolveSessionLoad() to avoid a config lookup for requests the shim can't handle, and fix ClientConfigurationManager to preserve session config when a diversion configuration is present even if the global session_load is 0. Change-Id: I3925b50b9ce0bd4e7b91b17888f9745d07dd9919

…e.useExperimental Change-Id: I0501f16a70e7de8c3d9b7df3cb8dcb95040ab07c

mutianf · 2026-10-18T02:15:52Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces support for session diversion configurations in DivertingUnaryCallable, enabling both global and per-scope diversion strategies (e.g., routing specific Bigtable methods like ReadRow or MutateRow based on a per-method session load). It also updates ClientConfigurationManager to preserve session configurations when a diversion configuration is present and adds comprehensive unit tests to verify the routing behavior. There are no review comments provided, and the changes look solid, so I have no additional feedback to provide.

…obalSessionLoad flag ReadRow/MutateRow fall back to flat session_load during server rollout; CAM/RMW are server-controlled via SessionDiversionConfiguration scope keys and never activate from flat session_load. Removes WipFeatures class. Change-Id: I4ab2cee5afacec930cdc44670b71c972a10fc660

alexoneill · 2026-10-19T17:48:18Z

+        .map(
+            c -> {
+              SessionClientConfiguration session = c.getSessionConfiguration();
+              return session.getSessionLoad() > 0 || session.hasSessionDiversionConfiguration();


Check that there is an entry in the map that has >0 value?

hasSessionDiversionConfiguration() only tests for presence, so a diversion config whose loads are all 0 would survive normalization and make areSessionsRequired() advertise session support for a client that routes nothing. Check for an actual nonzero load instead -- the flat session_load, global_diversion.session_load, or any per-scope entry. Change-Id: Id07d0dc0c83b7e5bb69294ff67d7f3c0eb4df2d7

nimf

Left a few comments, also PR description says

Moves the supports() check before resolveSessionLoad() to avoid a config lookup for requests the shim can't handle.

but this was reverted in one of the commits.

nimf · 2026-10-20T23:39:11Z

+   */
+  private static boolean divertsAnyTrafficToSessions(SessionClientConfiguration session) {
+    if (session.getSessionLoad() > 0) {
+      return true;


why are we returning true here immediately? What if session_load for ReadRow and MutateRow (or globally) is then configured to 0 in session diversion config?

it just short circut the check in nomarlize config.

nimf · 2026-10-22T17:54:15Z

  public boolean areSessionsRequired() {
-    return overrideConfig.map(c -> c.getSessionConfiguration().getSessionLoad() > 0).orElse(false);
+    return overrideConfig
+        .map(c -> divertsAnyTrafficToSessions(c.getSessionConfiguration()))


divertsAnyTrafficToSessions is also used here, and areSessionsRequired is used in ShimImpl which would set setSessionRequired on feature flags. Wouldn't that be an issue?

I think this is fine, because this uses override config to bypass session compatibility check. And it seems to be redundant because base feature flag already sets it to true. We should probably clean it up in a follow up PR.

…sion The default client config expressed "no session traffic" through the deprecated flat session_load. Replace it with an explicit per-scope diversion config covering the two methods that field governed, Bigtable.ReadRow and Bigtable.MutateRow, both at load 0. No behavior change: normalizeConfig only patches session_pool_configuration, channel_configuration, polling and debug_tag_level from the default, never the diversion config, so a server-returned config is unaffected. The pre-server config still resolves to no session traffic. Change-Id: Ifa78666b1edcbbddf61c065bb88ade69ef9bb44d

🤖 I have created a release *beep* *boop* --- <details><summary>1.94.0</summary> ## [1.94.0](v1.93.0...v1.94.0) (2026-10-22) ### Features * **auth:** request certificate-bound tokens for agent identities by ([01eaba1](01eaba1)) * **bigquery-jdbc:** Add clearcut telemetry client ([#14607](#14607)) ([c702438](c702438)) * **bigquery-jdbc:** expose QueryStatistics for jobless queries ([#14596](#14596)) ([15f472c](15f472c)) * **bigquery:** expose QueryStatistics from TableResult ([#14588](#14588)) ([40b2b1a](40b2b1a)) * **bigtable:** add per-method session diversion configuration ([#14681](#14681)) ([042dd3e](042dd3e)) * **bigtable:** support SessionCheckAndMutateRow on the session client ([#14565](#14565)) ([86de9c1](86de9c1)) * **bigtable:** support SessionReadModifyWriteRow on the session client ([#14585](#14585)) ([8e653b2](8e653b2)) * **firestore:** DML ([#14703](#14703)) ([11bc071](11bc071)) * **gax:** implement operation-level client request spans (T3) ([#14549](#14549)) ([275063b](275063b)) * **gax:** link attempt spans to parent context and clean up on operation completion ([#14459](#14459)) ([0c481bd](0c481bd)) * **gax:** standardize mapping terminal resumable upload exceptions to ApiException ([#14624](#14624)) ([d4961f0](d4961f0)) * **google/devicesandservices/health/v4beta:** add health v4beta ([#14600](#14600)) ([95f863c](95f863c)) * **google/shopping/merchant/reviews/v1:** add shopping-merchant-reviews ([#14724](#14724)) ([13310c7](13310c7)) * include writerId as part of trace id ([#13488](#13488)) ([ffa8063](ffa8063)) * log actionable errors on operation failure in LoggingTracer ([#14460](#14460)) ([27559f4](27559f4)) * populate client_stats in AppendRowsRequest ([#14614](#14614)) ([a7a0098](a7a0098)) * **spanner:** add option to create GcpFallbackChannel per channel instead of per pool ([#14501](#14501)) ([74f7591](74f7591)) ### Bug Fixes * **api-common:** validate path template variables against dot segments ([#14641](#14641)) ([4ad59e2](4ad59e2)) * **auth:** fix serialization gaps in DownscopedCredentials and UrlIdentityPoolSubjectTokenSupplier ([#14341](#14341)) ([a3e3324](a3e3324)) * **bigquery-jdbc:** ensure JDBC uses PQC-compliant algorithm by default ([#14560](#14560)) ([b0ffe3d](b0ffe3d)) * **bigquery-jdbc:** fix Exception handling ([#14566](#14566)) ([3e7a206](3e7a206)) * **bigquery-jdbc:** fix missing logger instances ([#14733](#14733)) ([0e822dc](0e822dc)) * **bigquery-jdbc:** fix pqc working in proxy env ([#14599](#14599)) ([ceb45b4](ceb45b4)) * **bigquery-jdbc:** format nested ARRAY fields as valid JSON in Struct.toString() ([#14720](#14720)) ([bc0126a](bc0126a)) * **bigquery-jdbc:** format TIMESTAMP as UTC in getString and add multi-zone TIMESTAMP(12) tests ([#14666](#14666)) ([e666612](e666612)) * **bigquery-jdbc:** honor `Calendar` timezone and prevent `ClassCastException` in ResultSet temporal getters ([#14657](#14657)) ([7dd2b37](7dd2b37)) * **bigquery-jdbc:** limit Storage Read API Arrow batch queue to 32 batches ([#14709](#14709)) ([d339c41](d339c41)) * **bigquery-jdbc:** normalize `setTime` from Calendar zone to JVM zone ([#14660](#14660)) ([54d2bc6](54d2bc6)) * **bigquery-jdbc:** poll the JSON reference queue and release processed finalizers ([#14719](#14719)) ([1b7d00d](1b7d00d)) * **bigquery-jdbc:** preserve `Timestamp` instant in `PreparedStatement` and `CallableStatement` Calendar overloads ([#14640](#14640)) ([3b02a44](3b02a44)) * **bigquery-jdbc:** preserve the instant on the TIMESTAMP write path and bind LocalDateTime as STRING ([#14647](#14647)) ([2106e26](2106e26)) * **bigquery-jdbc:** stop per-ResultSet logger registration leak ([#14718](#14718)) ([e83c768](e83c768)) * **bigquery-jdbc:** stop producer and release resources on ResultSet.close ([#14712](#14712)) ([6b99452](6b99452)) * **bigquery:** fix invalid mappings in domain models ([#14646](#14646)) ([17048ed](17048ed)) * **bigquery:** remove duplicate google-cloud-storage dependency ([#14586](#14586)) ([ab6173e](ab6173e)) * **datastore:** allow any gRPC TransportChannelProvider in DatastoreOptions ([#14478](#14478)) ([d9a8db5](d9a8db5)) * **datastore:** update emulator zip checksum for 2.3.1 ([#14592](#14592)) ([fb73ed5](fb73ed5)) * **deps:** update dependency com.google.api-client:google-api-client-bom to v2.9.2 ([#14725](#14725)) ([1ffa467](1ffa467)) * **gax-httpjson:** gracefully handle error details with unrecognized types ([#14612](#14612)) ([7295640](7295640)) * revise health metrics inflightBytes to become inflightBytesMax ([#14613](#14613)) ([44a12ef](44a12ef)) * **showcase:** update parent context retry tests for operation span in ITOtelTracing ([#14673](#14673)) ([312b5ea](312b5ea)) * **spanner:** increase max inbound message size from 256MB to 300MB ([#14551](#14551)) ([70c7439](70c7439)) * **spanner:** increase max inbound message size from 256MB to 300MB as ([70c7439](70c7439)) * **spanner:** prevent stream leaks and race conditions on result set close ([#14665](#14665)) ([013b49b](013b49b)) * **spanner:** support autoscaling config, edition, and positive capacity in cloud instance actions ([#14574](#14574)) ([004f0fd](004f0fd)) ### Performance Improvements * **bigquery:** defer blocking jobs.get RPC in QueryPageFetcher until getNextPage() ([#14650](#14650)) ([6183b63](6183b63)) ### Dependencies * Upgrade jackson-bom to v2.22.4 ([#14701](#14701)) ([09ce4c5](09ce4c5)) ### Documentation * **auth:** clarify quota project behavior on ImpersonatedCredentials ([#14597](#14597)) ([4e5a0d0](4e5a0d0)) * **pubsub:** Create a Cloud Pub/Sub sample for publishing with hedging ([#14623](#14623)) ([9de611f](9de611f)) * **pubsub:** regenerate README.md sample table ([#14643](#14643)) ([5394618](5394618)), refs [#14543](#14543) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

…#14738) Follow-up to #14681, from reviewing it after it merged. Three independent fixes. ### 1. Two drifting resolution rules (the main one) `DivertingUnaryCallable.resolveSessionLoad` (per RPC) and `ClientConfigurationManager.divertsAnyTrafficToSessions` (client-wide) answered "does traffic go to sessions?" with separately maintained rules that had already diverged: | | per-RPC | client-wide | |---|---|---| | diversion config present | authoritative, flat load ignored | ORs with flat `session_load` | | per-scope map | only this method's key | **any** key, incl. methods the client never serves | So `per_scope_diversion { "method:Bigtable.MutateRows": {session_load: 1.0} }` made `areSessionsRequired()` true — flipping the `sessions_required` / `sessions_compatible` feature flags sent to the server and keeping the session config and channel pool alive — while `DivertingUnaryCallable` diverted exactly zero requests, because no callable asks for that scope. Same for `session_load: 1.0` plus a CheckAndMutateRow-only map. Both now go through a new `SessionScope` enum, which owns the four scopes this client serves, whether each honours the deprecated flat `session_load`, and the single resolution rule (moved verbatim out of `DivertingUnaryCallable`). `ShimImpl` passes `SessionScope.READ_ROW` instead of a `String` + `boolean` pair. ### 2. The default config shadowed the flat `session_load` `bigtable-default-client-config.textproto` carried a zero-valued `per_scope_diversion` for ReadRow and MutateRow. Since a diversion configuration is authoritative once present, those entries forced **every** scope to 0 in any config derived from the default: - the initial `normalizeConfig(defaultConfig)` before the first `GetClientConfiguration` response, - the `handleFailedFetch` → `setClientConfiguration(defaultConfig)` path when the validity window expires during a config-service outage, - anything merged from the `bigtable.internal.client-config-override` sys-prop, including the `bigtable-session-it` Maven profile. This surfaced as 10 `ClientConfigurationManagerTest` failures the moment fix 1 made the two checks agree — they had been passing only because the client-wide check disagreed and reported sessions as on. The entries were purely documentary (an absent key and an absent diversion config both resolve to 0), so they are removed. ### 3. ReadModifyWriteRow could return `null` on the session path The extractor was `r -> r.hasRow() ? r.getRow() : null`, so a `SessionReadModifyWriteRowResponse` without a row handed user code a `null` the classic path never produces (`EnhancedBigtableStub` always builds a `Row` from the possibly-unset `resp.getRow()`). Unlike ReadRow — where a missing key legitimately means `null` — ReadModifyWriteRow always mutates and always yields a row, so it now extracts unconditionally. Both extractors are named constants with tests pinning the split. Also forwards the per-method retry settings to the default call context for CheckAndMutateRow and ReadModifyWriteRow, matching the existing ReadRow and MutateRow wiring. ### Testing New `SessionScopeTest` (7 cases) covers the resolution rule and the agreement invariant; new cases in `ShimImplTest` pin the ReadRow-null / ReadModifyWriteRow-non-null split, and `ClientConfigurationManagerTest` gains one asserting the bundled default never shadows a flat `session_load`. `DivertingUnaryCallableTest`, `ClientConfigurationManagerTest`, `ShimImplTest`, `SessionDeadlineTest`, `SessionScopeTest`, `RowBuilderShimTest`, `ReadModifyWriteRowShimTest` — 57 tests, all passing.

mutianf requested review from a team as code owners October 18, 2026 02:00

gemini-code-assist Bot reviewed Oct 18, 2026

View reviewed changes

mutianf force-pushed the feat/session-diversion-config branch from 56b775f to cc0ad8f Compare October 18, 2026 02:11

mutianf changed the title ~~feat(bigtable): add session diversion for ReadRow, MutateRow, CheckAndMutateRow, and ReadModifyWriteRow~~ feat(bigtable): add per-method session diversion configuration Oct 18, 2026

fix(bigtable): check ratio before supports() in DivertingUnaryCallabl…

21d9342

…e.useExperimental Change-Id: I0501f16a70e7de8c3d9b7df3cb8dcb95040ab07c

gemini-code-assist Bot reviewed Oct 18, 2026

View reviewed changes

mutianf force-pushed the feat/session-diversion-config branch from ef49486 to 1d37bde Compare October 19, 2026 17:21

alexoneill reviewed Oct 19, 2026

View reviewed changes

nimf reviewed Oct 21, 2026

View reviewed changes

add client context

7678fac

nimf reviewed Oct 22, 2026

View reviewed changes

nimf approved these changes Oct 22, 2026

View reviewed changes

mutianf merged commit 042dd3e into googleapis:main Oct 22, 2026
206 checks passed

mutianf deleted the feat/session-diversion-config branch October 22, 2026 20:57

mutianf mentioned this pull request Oct 22, 2026

fix(bigtable): resolve session diversion through a single shared rule #14738

Merged

release-please Bot mentioned this pull request Oct 22, 2026

chore(main): release 1.94.0 #14723

Merged

Conversation

mutianf commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mutianf commented Oct 18, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

alexoneill Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

nimf left a comment

Choose a reason for hiding this comment

Uh oh!

nimf Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

mutianf Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nimf Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

mutianf Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mutianf commented Oct 18, 2026 •

edited

Loading