perf(spanner): remove rest-parameter allocations in wrappedRequestFn - #9365
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the wrappedRequestFn helper in the Spanner client to robustly inject the Spanner request ID into errors across callbacks, promises, streams, and synchronous exceptions, while preserving the cancel method on cancellable promises. It also adds comprehensive unit and mock server tests to verify these error-handling behaviors. Feedback on the changes suggests adding a truthiness guard on the callback's error argument before passing it to injectRequestIDIntoError to prevent potential runtime exceptions when operations succeed.
7ebedcd to
a7657e8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the wrappedRequestFn in prepareGapicRequest_ to decouple error enrichment from the caller's mutable config object by extracting a lightweight errorConfig containing only headers. It also improves error handling across callback, promise, and stream modes to reliably inject the Spanner request ID, and adds comprehensive unit and mock server tests to verify these behaviors. The review feedback suggests reverting the manual backup and restoration of process.env in the tests to Sinon's sandbox.stub to prevent potential test pollution and flakiness.
a7657e8 to
0512080
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the error-wrapping mechanism in prepareGapicRequest_ to robustly inject Spanner request IDs into errors across callbacks, promises, streams, and synchronous exceptions, while decoupling the configuration and preserving promise cancellation. It also adds comprehensive unit and mock server tests to validate these scenarios. As there are no review comments, we have no additional feedback to provide.
Every RPC passes through wrappedRequestFn in prepareGapicRequest_. The function previously used rest parameters (...args) and sliced the arguments array on each call, creating two ephemeral arrays per RPC. Because reqOpts and gaxOpts are already bound to requestFn, wrappedRequestFn only receives an optional callback. This change: - Replaces ...args and args.slice with a single optional callback parameter. - Reuses the initial promise in non-callback mode instead of invoking requestFn a second time. - Preserves .cancel() when requestFn returns a CancellablePromise. - Adds try/catch in callback mode to inject the request ID if requestFn throws synchronously.
0512080 to
156dc83
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors client instantiation and error handling in the Spanner client, decoupling error enrichment from mutable config objects and ensuring Spanner request IDs are correctly injected across callbacks, promises, and streams. It also adds comprehensive unit and mock tests to verify these behaviors. Feedback suggests reverting the promise chain refactoring from .then().catch() back to .then(onFulfilled, onRejected) to avoid allocating an extra promise, maintaining the PR's focus on performance and reducing allocations.
Every RPC passes through wrappedRequestFn in prepareGapicRequest_. The function previously used rest parameters (...args) and sliced the arguments array on each call, creating two ephemeral arrays per RPC. Because reqOpts and gaxOpts are already bound to requestFn, wrappedRequestFn only receives an optional callback.
This change: