Repository navigation
Conversation
![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 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.
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.
126a636 to
6529120
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors AsyncResultSetImpl to implement a non-blocking cooperative model for streaming data from Spanner. It introduces an isDataAvailable() method across various result set implementations to allow non-blocking checks for data availability and updates test suites to cover these changes. The reviewer identified a thread-safety issue regarding concurrent or duplicate calls to close() on the delegate ResultSet and provided a code suggestion to ensure the closing logic is idempotent.
6529120 to
91b7bac
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous streaming and concurrency model in the Cloud Spanner Java client. It introduces a non-blocking isDataAvailable() check across the result set hierarchy and replaces the blocking latch-based synchronization in AsyncResultSetImpl with a state-driven scheduling mechanism for producers and callbacks. Additionally, it hardens lifecycle management and thread safety in ResumableStreamIterator and adds extensive test coverage. Feedback is provided regarding a potential resource leak in AsyncResultSetImpl.close() when called while a query is actively running, suggesting that the delegate result set should be closed and the state transitioned to CANCELLED regardless of the current state.
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.
91b7bac to
b413248
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a non-blocking isDataAvailable() mechanism across various Spanner ResultSet implementations and refactors the asynchronous streaming and concurrency model in AsyncResultSetImpl and ResumableStreamIterator to use explicit state tracking instead of blocking CountDownLatches. Feedback on these changes highlights a potential resource leak in AsyncResultSetImpl.close() where the delegate ResultSet is not closed immediately if the state is RUNNING, CONSUMING, or PAUSED, along with a potential NullPointerException during service shutdown. Additionally, a dead local variable shouldScheduleProducer and its corresponding conditional block in CallbackRunnable were identified for removal.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous result set streaming implementation to use a non-blocking, scheduler-driven state machine instead of blocking threads on CountDownLatch objects. It introduces an isDataAvailable() check across the result set and iterator hierarchy to determine if data can be read immediately without blocking on network I/O. Additionally, it improves thread safety during stream lifecycle events (such as concurrent cancellation, closure, and resumption) in ResumableStreamIterator using explicit locks and state flags. Comprehensive unit and integration tests are added to validate these concurrency and streaming improvements. I have no feedback to provide as there are no review comments.
…riven AsyncResultSet Refines the event-driven AsyncResultSet implementation and underlying stream iterators to eliminate potential deadlocks, race conditions, and inconsistent stream cancellation behavior: - Ensure ProduceRowsRunnable never blocks an executor thread on incomplete rows: GrpcValueIterator now non-blockingly buffers available chunks and verifies that a complete row (including rows split across multiple PartialResultSets or chunked values) is in memory before next() is called. - Simplify the AsyncResultSetImpl state machine to use a single monitor lock with out-of-lock task scheduling and cleanup, eliminating redundant state flags and reaching 100% test coverage. - Make gRPC stream cancellation deterministic and consistent between ResultSet and AsyncResultSet: the underlying gRPC call is cleared once the PartialResultSet with last=true is consumed from the stream queue, and is cancelled if the result set is closed before that chunk is consumed. - Prevent a queue-full deadlock in GrpcStreamIterator when last=true is followed by onCompleted or onError with prefetchChunks=1 by ensuring END_OF_STREAM is enqueued at most once. - Guard against concurrent close() and early message arrival during stream and inlined transaction initialization in ResumableStreamIterator, GrpcStreamIterator, AbstractReadContext, and TransactionRunnerImpl.
613abb1 to
413bfb5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces non-blocking data availability checks and refactors stream state management in the Google Cloud Spanner Java client. Key changes include adding isDataAvailable() and canPollWithoutBlocking() across result set implementations, replacing latch-based synchronization in AsyncResultSetImpl with a robust scheduling model, and improving thread safety during stream cancellation and retries. The review feedback suggests wrapping the nullification of callback and executor in a synchronized block to prevent data races, optimizing getDelegate() calls in AsyncResultSetImpl.next(), and removing redundant fully qualified class names in the tests.
413bfb5 to
dc90f60
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a non-blocking data availability check mechanism across the Spanner result set stack by adding isDataAvailable() and canPollWithoutBlocking() methods. It refactors the threading and synchronization model in AsyncResultSetImpl to replace latch-based pausing with explicit scheduling, improves thread safety across iterators using volatile fields and synchronized blocks, and robustly handles early cancellation and failures of inlined begin transactions. Extensive unit and integration tests have been added to verify these changes. Since there are no review comments, I have no feedback to provide.
This branch has not been deployed
1 participant
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.