perf(spanner): make internal AsyncResultSet row production non-blocking - #14268
perf(spanner): make internal AsyncResultSet row production non-blocking#14268olavloite wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a non-blocking isDataAvailable() check across Spanner result set classes to determine if data can be read immediately without blocking on network I/O. It refactors AsyncResultSetImpl to use a state-machine-based scheduling mechanism for producers and callbacks, replacing the previous blocking latch-based synchronization. Additionally, comprehensive unit tests are added to verify these changes. The review feedback highlights that catching Throwable in ProduceRowsRunnable can swallow InterruptedException, and recommends restoring the thread's interrupted status if an InterruptedException is caught.
c74722a to
a2fc337
Compare
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous result set implementation (AsyncResultSetImpl) to support non-blocking streaming data checks via a new isDataAvailable() method. It replaces the previous latch-based synchronization mechanism with a state-driven scheduling model for producers and callbacks, and introduces strict null-safety annotations (@NullMarked and @Nullable). The review feedback highlights three critical issues in AsyncResultSetImpl where the nullable fields callback, executor, and result are dereferenced directly without local capture or null checks, which could lead to NullPointerExceptions and violates the newly introduced null-safety rules.
a2fc337 to
4e93618
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a non-blocking data availability check (isDataAvailable()) across various result set and iterator classes in the Google Cloud Spanner library. It also refactors the concurrency model in AsyncResultSetImpl by replacing several CountDownLatches with state flags and explicit scheduling methods to improve thread safety and prevent blocking. The feedback suggests replacing the synchronized blocks in AsyncResultSetImpl with explicit ReentrantLocks to avoid carrier thread pinning when using virtual threads.
4e93618 to
2052c6c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces non-blocking data availability checks (isDataAvailable()) across various result set and iterator implementations, including AsyncResultSetImpl, GrpcStreamIterator, GrpcValueIterator, and ResumableStreamIterator. Additionally, AsyncResultSetImpl is refactored to replace thread-blocking latches with state flags to coordinate producer and consumer threads asynchronously. The review feedback highlights critical race conditions in ResumableStreamIterator.java where gRPC streams could be leaked or started after the iterator is closed, and suggests introducing a closed flag to safely manage the lifecycle of the iterator and its streams.
Refactors AsyncResultSetImpl to produce rows cooperatively without holding executor threads. Uses isDataAvailable on underlying streaming iterators to avoid blocking on network I/O or full buffers, eliminating thread-per-query pinning and improving throughput under concurrency.
2052c6c to
126a636
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous result set implementation (AsyncResultSetImpl) and stream iterators (ResumableStreamIterator, GrpcStreamIterator, etc.) to introduce a non-blocking, event-driven execution model. By implementing a new isDataAvailable() check across the result set hierarchy, the producer thread no longer blocks on network I/O when waiting for stream data, preventing thread starvation in single-threaded or limited executor pools. Additionally, resource cleanup, cancellation safety, and exception propagation have been significantly hardened, backed by comprehensive unit and integration tests. There are no review comments to assess, and the changes look solid.
Refactors AsyncResultSetImpl to produce rows cooperatively without holding executor threads. Uses isDataAvailable on underlying streaming iterators to avoid blocking on network I/O or full buffers, eliminating thread-per-query pinning and improving throughput under concurrency.