Conversation
…mption Replace the setTimeout backpressure polling loop in PartialResultStream with native Node.js stream flow control: - Hold the transform completion callback when downstream push() returns false, and invoke it upon stream drain via _read() to resume upstream ingestion. - Remove _tryResume, _numPushFailed, and the artificial 'Stream is still not ready to receive data' timeout failures. - Guard 'paused' event emission in _addChunk so it only emits once per backpressure transition instead of on every subsequent value. - Mark maxResumeRetries as deprecated on streaming RowOptions, clarifying that it is only used by non-streaming Snapshot.run() for RPC retry limits. - Replace setTimeout with setImmediate in test/spanner.ts slow-writer test, eliminating a 5-second sleep in compliance with the zero-sleep test policy. - Add comprehensive backpressure unit tests covering single-chunk direct decoding, multi-chunk pause/resume cycles, cross-chunk value stitching, stream destroy/error propagation, and full-pipeline flow control.
There was a problem hiding this comment.
Code Review
This pull request replaces the timeout-based retry mechanism for stream backpressure with automatic, event-driven stream flow control in PartialResultStream, deprecating the maxResumeRetries option for streaming queries. Feedback on the changes suggests wrapping super._read(size) in an else block inside the _read method to avoid potential redundant calls to the parent class's read implementation when resuming from a paused state.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors backpressure handling in PartialResultStream to use native Node.js stream flow control instead of a timer-based retry mechanism. The maxResumeRetries option is deprecated for streaming queries, and the stream now pauses and resumes automatically using _read() and a cached callback when downstream buffers are full. Comprehensive unit tests have been added to verify this event-driven backpressure behavior, and existing tests have been updated to reflect that slow writers no longer trigger failures. There are no review comments to address, and the implementation looks solid.
Replace the setTimeout backpressure polling loop in PartialResultStream with native Node.js stream flow control: