chore(bigquerystorage): fix flaky ConnectionWorkerPoolTest#13662
Merged
Conversation
Fix ConnectionWorkerPoolTest.testMultiStreamAppend_appendWhileClosing by limiting max connections per region to 5. This prevents the pool from scaling up to 6 connections due to timing-dependent overwhelm of mock connections, when the test expects exactly 5. Also fix a concurrency issue in FakeBigQueryWriteImpl where requestReceivedInstants ArrayList was accessed concurrently without synchronization, causing ArrayIndexOutOfBoundsException (reported as UNKNOWN RPC error in tests). Fixes #13637 TAG=agy CONV=79e14d4e-c762-4dad-a0d6-de0dd0e3d3f6
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses concurrency and timing issues in tests by limiting maximum connections in ConnectionWorkerPoolTest and synchronizing access to requestReceivedInstants in FakeBigQueryWriteImpl. However, the feedback highlights that other shared variables in FakeBigQueryWriteImpl, such as recordCount and requests, are still modified concurrently without synchronization, which could lead to race conditions or exceptions.
Improve thread safety of FakeBigQueryWriteImpl by: - Replacing requestReceivedInstants ArrayList with CopyOnWriteArrayList and removing manual synchronization. - Replacing recordCount, connectionCount, expectedOffset (long) and responseIndex (int) with AtomicLong/AtomicInteger to avoid race conditions and lost updates. - Synchronizing the offset verification block to ensure atomic check-and-increment of expectedOffset. This addresses feedback on PR #13662 regarding potential concurrency issues in the test fake. TAG=agy CONV=79e14d4e-c762-4dad-a0d6-de0dd0e3d3f6
…fset verification Replace the synchronized block in FakeBigQueryWriteImpl offset verification with AtomicLong.compareAndSet. This makes the check-and-increment atomic and lock-free, improving performance and thread safety for concurrent streams. Added comments explaining the approach. TAG=agy CONV=79e14d4e-c762-4dad-a0d6-de0dd0e3d3f6
jinseopkim0
approved these changes
Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix ConnectionWorkerPoolTest.testMultiStreamAppend_appendWhileClosing by limiting max connections per region to 5. This prevents the pool from scaling up to 6 connections due to timing-dependent overwhelm of mock connections, when the test expects exactly 5.
Also fix a concurrency issue in FakeBigQueryWriteImpl where requestReceivedInstants ArrayList was accessed concurrently without synchronization, causing ArrayIndexOutOfBoundsException (reported as UNKNOWN RPC error in tests).
Fixes #13637