Conversation
olavloite
commented
Sep 14, 2026
olavloite
commented
- Precomputes 'x-goog-spanner-route-to-leader: true' once in Transaction and PartitionedDml constructors when routeToLeaderEnabled is true.
- Removes redundant per-request header mutations and option checks across Snapshot and Transaction methods (begin, read, runStream, batchUpdate, commit, rollback).
- Uses safe navigation in Snapshot.prototype._getSpanner to support lightweight sessions without full parent chains.
- Fixes latent in-place mutation in Database.prototype.batchCreateSessions where database.commonHeaders_ was mutated, which caused subsequent read-only snapshots to inadvertently route to the leader.
- Passes a shallow copy of commonHeaders_ in Transaction.prototype.rollback to protect against downstream in-place header modifications.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how leader-aware routing headers are applied to Spanner requests. Instead of injecting these headers dynamically during individual operations (such as begin, read, query, commit, and rollback), they are now precomputed in the constructors of Transaction and PartitionedDml when routeToLeaderEnabled is active. Additionally, _getSpanner() has been updated to safely navigate the session hierarchy, and extensive unit tests have been added to verify header behavior. However, the reviewer identified a critical issue in both Transaction and PartitionedDml constructors: mutating this.commonHeaders_ directly will mutate the shared, pooled session.commonHeaders_ reference. This would cause subsequent read-only snapshots using the same session to incorrectly inherit the route-to-leader header. To resolve this, this.commonHeaders_ should be shallow-copied before applying the leader-aware routing header.
…_ mutation - Precomputes 'x-goog-spanner-route-to-leader: true' once in Transaction and PartitionedDml constructors when routeToLeaderEnabled is true. - Removes redundant per-request header mutations and option checks across Snapshot and Transaction methods (begin, read, runStream, batchUpdate, commit, rollback). - Uses safe navigation in Snapshot.prototype._getSpanner to support lightweight sessions without full parent chains. - Fixes latent in-place mutation in Database.prototype.batchCreateSessions where database.commonHeaders_ was mutated, which caused subsequent read-only snapshots to inadvertently route to the leader. - Passes a shallow copy of commonHeaders_ in Transaction.prototype.rollback to protect against downstream in-place header modifications.
b3fb625 to
b1f43e9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors how leader-aware routing headers are managed and applied to requests in the Spanner client. Instead of repeatedly adding the header on individual operations (which risked mutating shared commonHeaders_), the header is now precomputed and added to commonHeaders_ during the initialization of Transaction and PartitionedDml when routeToLeaderEnabled is true. For Database.batchCreateSessions, the header is safely added to the request-specific headers instead of mutating commonHeaders_. Additionally, optional chaining is introduced in _getSpanner() to safely retrieve the Spanner instance. Comprehensive unit tests have been added to verify these changes. There are no review comments, so I have no feedback to provide.
1 participant
