feat(spanner): make affinity keys configurable via spanner_grpc_confi…#8157
feat(spanner): make affinity keys configurable via spanner_grpc_confi…#8157alkatrivedi wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
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.
77436fe to
ec16d7c
Compare
7ac6c1a to
af07c0f
Compare
2ed0b4e to
9343cb2
Compare
9343cb2 to
6c14326
Compare
bf6519e to
f0d2172
Compare
cd36e7e to
fa90d56
Compare
d7da300 to
c894f4c
Compare
65fe70e to
2070d68
Compare
2070d68 to
e820c2b
Compare
…eapis/google-cloud-node into configurable-affinity-keys
…eapis/google-cloud-node into configurable-affinity-keys
…eapis/google-cloud-node into configurable-affinity-keys
…eapis/google-cloud-node into configurable-affinity-keys
|
/gemini review |
There was a problem hiding this comment.
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.
| 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); | ||
| }; |
There was a problem hiding this comment.
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);
};| const database = this.session.parent as Database; | ||
| const spanner = database.parent.parent as Spanner; | ||
| const client = spanner.clients_.get('SpannerClient') as any; |
There was a problem hiding this comment.
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.
| 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; |
|
/gcbrun |
No description provided.