chore: increase awaitTermination timeout in ITBulkConnectionTest#4375
chore: increase awaitTermination timeout in ITBulkConnectionTest#4375sakthivelmanii wants to merge 3 commits intomainfrom
Conversation
70dbc58 to
5eb9228
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an intermittent test failure in Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an intermittent test failure in ITBulkConnectionTest by increasing the awaitTermination timeout and adding an assertion to ensure the executor terminates successfully. The change is correct and improves the test's reliability. I have one suggestion to improve maintainability by extracting the new timeout value into a constant.
| } | ||
| executor.shutdown(); | ||
| executor.awaitTermination(10L, TimeUnit.SECONDS); | ||
| assertThat(executor.awaitTermination(60L, TimeUnit.SECONDS), is(true)); |
There was a problem hiding this comment.
To improve readability and maintainability, it would be best to extract the magic number 60L into a named constant at the class level. This makes the purpose of the value clear and simplifies future modifications.
For example:
private static final long AWAIT_TERMINATION_TIMEOUT_SECONDS = 60L;And then use it in the assertion:
assertThat(executor.awaitTermination(AWAIT_TERMINATION_TIMEOUT_SECONDS, TimeUnit.SECONDS), is(true));
This PR fixes an intermittent failure in
ITBulkConnectionTestcaused by the test threads not finishing completely before the test concludes.The
testBulkCreateConnectionsMultiThreadedtest shuts down the thread pool and then waits for the threads to finish usingawaitTermination(10L, TimeUnit.SECONDS). Under load, or in constrained environments, 10 seconds is sometimes insufficient for 250 connections to be opened, queried, and closed across 50 background threads. If the wait times out, the test proceeds to callcloseSpanner(), which checks the connection pool. Because some of the original threads are still running and haven't calledclose()on their connections yet,closeSpanner()throws aFAILED_PRECONDITION: There is/are X connection(s) still openexception.This PR increases the
awaitTerminationtimeout to60seconds and adds an assertion constraint to prevent the test from falling through.Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes test failure ☕️
If you write sample code, please follow the samples format.