Skip to content

fix(spanner): restore setCall overload for stream test compatibility - #14885

Merged
sakthivelmanii merged 1 commit into
mainfrom
spanner-restore-set-call-overload
Nov 2, 2026
Merged

sakthivelmanii merged 1 commit into
mainfrom
spanner-restore-set-call-overload

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

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.

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.
@olavloite
olavloite requested review from a team as code owners November 2, 2026 10:30

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +161 to +163
synchronized (this) {
this.withBeginTransaction = withBeginTransaction;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can be simplified, then we will do so in a follow-up PR (currently under investigation).

@sakthivelmanii
sakthivelmanii enabled auto-merge (squash) November 2, 2026 10:45
@sakthivelmanii
sakthivelmanii merged commit cfe728e into main Nov 2, 2026
201 checks passed
@sakthivelmanii
sakthivelmanii deleted the spanner-restore-set-call-overload branch November 2, 2026 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants