Repository navigation
fix(spanner): restore setCall overload for stream test compatibility - #14885
Conversation
Merging PR #14770 and PR #14776 concurrently broke the build on main. One PR added unit tests that configure mock streams using a two-argument setCall method to indicate whether a transaction is inlined, while the other PR removed that parameter because production code now configures this during stream construction. This change restores the two-argument setCall method for testing and keeps the inlined transaction state configurable during test setup.
There was a problem hiding this comment.
Code Review
This pull request modifies GrpcStreamIterator.java by changing the withBeginTransaction field from final to volatile and adding a new @VisibleForTesting overload of setCall to allow updating this field. The feedback suggests that wrapping the assignment of the volatile field in a synchronized block is redundant and can be simplified.
| synchronized (this) { | ||
| this.withBeginTransaction = withBeginTransaction; | ||
| } |
There was a problem hiding this comment.
The withBeginTransaction variable is declared as volatile, which already guarantees visibility and atomicity for writes. If this assignment does not need to be atomic with other operations protected by synchronized (this), wrapping it in a synchronized block is redundant. Please verify whether the lock is required for coordinating other states before removing it.
this.withBeginTransaction = withBeginTransaction;References
- Before suggesting lock-free alternatives or atomic variables to avoid lock contention, verify whether the target method actually acquires any locks or if the state is already managed without synchronization.
There was a problem hiding this comment.
If it can be simplified, then we will do so in a follow-up PR (currently under investigation).
Merging PR #14770 and PR #14776 concurrently broke the build on main. One PR added unit tests that configure mock streams using a two-argument setCall method to indicate whether a transaction is inlined, while the other PR removed that parameter because production code now configures this during stream construction.
This change restores the two-argument setCall method for testing and keeps the inlined transaction state configurable during test setup.