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.
Moves the supports() check before resolveSessionLoad() to avoid a config lookup for requests the shim can't handle.
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-20T23:57:28Z

+        case PER_SCOPE_DIVERSION:
+          SessionScopeDiversionConfiguration scopeConfig =
+              diversionConfig.getPerScopeDiversion().getScopeDiversionsMap().get(methodScopeKey);
+          return scopeConfig != null ? scopeConfig.getSessionLoad() : 0f;


so here if we don't have ReadRow or MutateRow in the map we will return 0, but the comment for the old session_load in the proto says:

// Deprecated: Prefer session_diversion_configuration. If both are provided,
// the client should apply this session_load to Bigtable.ReadRow &
// Bigtable.MutateRow, then process the session_diversion_configuration,
// overwriting any behavior established by this value.

We don't apply the old global session_load for ReadRow and MutateRow here. Is the comment stale?

Yeah, will need to update the comment

nimf · 2026-10-21T00:30:41Z

+              diversionConfig.getPerScopeDiversion().getScopeDiversionsMap().get(methodScopeKey);
+          return scopeConfig != null ? scopeConfig.getSessionLoad() : 0f;
+        default:
+          return 0f;


We can get here only if SessionDiversionConfiguration is provided but with the strategy is not set or set to some new value in the future. Should we check fallbackToGlobalSessionLoad and fallback then?

No I think server sets one of these field. fallbackToGlobalSessionLoad is only for the transition period.

nimf · 2026-10-21T00:52:16Z

-    if (!WipFeatures.CHECK_AND_MUTATE_ROW_ENABLED) {
-      return classic;
-    }
    return new DivertingUnaryCallable<>(


now that we don't return classic anymore, we are missing adding withDefaultCallContext in the EnhancedBigtableStub that calls this.

nimf · 2026-10-21T00:52:29Z

-    if (!WipFeatures.READ_MODIFY_WRITE_ROW_ENABLED) {
-      return classic;
-    }
    return new DivertingUnaryCallable<>(


now that we don't return classic anymore, we are missing adding withDefaultCallContext in the EnhancedBigtableStub that calls this.

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

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!

Choose a reason for hiding this comment

Uh oh!

nimf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mutianf commented Oct 18, 2026 •

edited

Loading