fix(spanner): fix flakiness and race conditions in multiplexed session tests#12949
fix(spanner): fix flakiness and race conditions in multiplexed session tests#12949sakthivelmanii merged 1 commit intomainfrom
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 new helper method, ofMinimumAndRandomTimeAndStickyException, in MockSpannerServiceImpl to simulate RPC delays alongside persistent exceptions. Corresponding tests in MultiplexedSessionDatabaseClientMockServerTest were updated to utilize this method. Feedback suggests replacing the magic number 200 with a named constant for better clarity and considering a more targeted reset of execution times instead of using the broad removeAllExecutionTimes() method.
| mockSpanner.setCreateSessionExecutionTime( | ||
| SimulatedExecutionTime.ofStickyException(Status.DEADLINE_EXCEEDED.asRuntimeException())); | ||
| SimulatedExecutionTime.ofMinimumAndRandomTimeAndStickyException( | ||
| 200, 0, Status.DEADLINE_EXCEEDED.asRuntimeException())); |
There was a problem hiding this comment.
The magic number 200 is used here and in other places (e.g., line 342) to represent the simulated execution delay. It would be better to define this as a named constant (e.g., SIMULATED_RPC_DELAY_MS) to improve maintainability and clarity across the test suite.
| // The CreateSession RPC will be retried, and as the exception is removed by the first call, | ||
| // The CreateSession RPC will be retried, and as we manually clear the exception, | ||
| // the second attempt will succeed. | ||
| mockSpanner.removeAllExecutionTimes(); |
There was a problem hiding this comment.
The use of removeAllExecutionTimes() is quite broad as it clears all simulated execution times for all RPC methods. While safe in this specific test context, consider using a more targeted reset (e.g., setCreateSessionExecutionTime(null)) if available, to avoid unintended side effects in more complex test scenarios where other RPCs might have configured behaviors.
sakthivelmanii
force-pushed
the
fix-multiplexed-session-test
branch
2 times, most recently
from
e55d353 to
15b11ed
Compare
| } | ||
|
|
||
| private void maybeFreezeAndRecordRequest(AbstractMessage request) { | ||
| synchronized (lock) { |
There was a problem hiding this comment.
(Probably does not matter much, but still): Do you really need to take the lock here?
sakthivelmanii
force-pushed
the
fix-multiplexed-session-test
branch
from
15b11ed to
acc5b8c
Compare
3 participants
No description provided.