Skip to content

feat(spanner): make affinity keys configurable via spanner_grpc_confi…#8157

Open
alkatrivedi wants to merge 10 commits into
mainfrom
configurable-affinity-keys
Open

feat(spanner): make affinity keys configurable via spanner_grpc_confi…#8157
alkatrivedi wants to merge 10 commits into
mainfrom
configurable-affinity-keys

Conversation

@alkatrivedi

Copy link
Copy Markdown
Contributor

No description provided.

@alkatrivedi alkatrivedi requested a review from a team as a code owner May 3, 2026 00:58
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label May 3, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements multiplexed session affinity for Spanner by generating unique UUID-based affinity keys for transactions and snapshots. It updates the gRPC configuration to support metadata-based affinity and modifies the Snapshot and Transaction classes to inject these keys into request headers, as well as handling the unbinding of keys during commit or rollback operations. Feedback is provided regarding the potential risks of bypassing the grpcGcp.createGcpApiConfig helper, performance inefficiencies caused by repeated linear searches in the configuration, and the need to avoid direct mutation of request configuration objects to prevent side effects.

Comment thread handwritten/spanner/src/transaction.ts Outdated
Comment thread handwritten/spanner/src/transaction.ts
Comment thread handwritten/spanner/src/transaction.ts
Comment thread handwritten/spanner/src/transaction.ts Outdated
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch 2 times, most recently from 77436fe to ec16d7c Compare May 8, 2026 07:22
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch 3 times, most recently from 7ac6c1a to af07c0f Compare May 12, 2026 07:38
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch from 2ed0b4e to 9343cb2 Compare May 17, 2026 21:28
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch from 9343cb2 to 6c14326 Compare May 27, 2026 07:11
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch 5 times, most recently from bf6519e to f0d2172 Compare June 16, 2026 04:27
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch 12 times, most recently from cd36e7e to fa90d56 Compare June 22, 2026 18:56
@shivanee-p shivanee-p requested review from a team and removed request for a team June 22, 2026 21:15
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch from d7da300 to c894f4c Compare June 23, 2026 05:39
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch from 65fe70e to 2070d68 Compare June 23, 2026 06:08
@alkatrivedi alkatrivedi added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 23, 2026
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 23, 2026
@alkatrivedi alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jun 23, 2026
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 23, 2026
@googleapis googleapis deleted a comment from gemini-code-assist Bot Jun 23, 2026
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 23, 2026
@alkatrivedi alkatrivedi force-pushed the configurable-affinity-keys branch from 2070d68 to e820c2b Compare June 23, 2026 06:14
@alkatrivedi alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jun 23, 2026
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 23, 2026
@surbhigarg92

Copy link
Copy Markdown
Contributor

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for multiplexed session affinity keys in Spanner transactions and snapshots. It generates a unique affinity key for multiplexed sessions, injects it into request options, and ensures it is unbound when the transaction ends, commits, or rolls back. Unit tests have been added to verify this behavior. The reviewer feedback suggests avoiding mutation of the config object in request and requestStream by shallow copying it, and using optional chaining when accessing nested parent properties to prevent potential runtime TypeErrors.

Comment on lines +408 to +436
this.request = (config: any, callback: Function) => {
if (this._affinityKey) {
if (!config.gaxOpts || Object.keys(config.gaxOpts).length === 0) {
config.gaxOpts = this._bindGaxOpts as any;
} else {
config.gaxOpts = injectGaxOpt(
config.gaxOpts,
'affinityKey',
this._affinityKey,
);
}
}
return session.request(config, callback);
};

this.requestStream = (config: any) => {
if (this._affinityKey) {
if (!config.gaxOpts || Object.keys(config.gaxOpts).length === 0) {
config.gaxOpts = this._bindGaxOpts as any;
} else {
config.gaxOpts = injectGaxOpt(
config.gaxOpts,
'affinityKey',
this._affinityKey,
);
}
}
return session.requestStream(config);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Mutating the config object passed to this.request and this.requestStream can lead to unexpected side effects if the caller reuses the configuration object (e.g., during retries or multiple sequential requests). It is safer to shallow copy the config object before modifying its gaxOpts property.

    this.request = (config: any, callback: Function) => {
      if (this._affinityKey) {
        let gaxOpts;
        if (!config.gaxOpts || Object.keys(config.gaxOpts).length === 0) {
          gaxOpts = this._bindGaxOpts as any;
        } else {
          gaxOpts = injectGaxOpt(
            config.gaxOpts,
            'affinityKey',
            this._affinityKey,
          );
        }
        config = Object.assign({}, config, {gaxOpts});
      }
      return session.request(config, callback);
    };

    this.requestStream = (config: any) => {
      if (this._affinityKey) {
        let gaxOpts;
        if (!config.gaxOpts || Object.keys(config.gaxOpts).length === 0) {
          gaxOpts = this._bindGaxOpts as any;
        } else {
          gaxOpts = injectGaxOpt(
            config.gaxOpts,
            'affinityKey',
            this._affinityKey,
          );
        }
        config = Object.assign({}, config, {gaxOpts});
      }
      return session.requestStream(config);
    };

Comment on lines +1100 to +1102
const database = this.session.parent as Database;
const spanner = database.parent.parent as Spanner;
const client = spanner.clients_.get('SpannerClient') as any;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Accessing nested properties like this.session.parent and database.parent.parent without optional chaining can cause a TypeError at runtime if any of these parent references are undefined (e.g., in lightweight test environments or mocked sessions). Using optional chaining ensures defensive programming and avoids potential crashes.

Suggested change
const database = this.session.parent as Database;
const spanner = database.parent.parent as Spanner;
const client = spanner.clients_.get('SpannerClient') as any;
const database = this.session?.parent as Database;
const spanner = database?.parent?.parent as Spanner;
const client = spanner?.clients_?.get('SpannerClient') as any;

@alkatrivedi

Copy link
Copy Markdown
Contributor Author

/gcbrun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API. kokoro:force-run Add this label to force Kokoro to re-run the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants