feat(bigquery-jdbc): support connection-scoped executor isolation and dynamic queue warnings#13413
feat(bigquery-jdbc): support connection-scoped executor isolation and dynamic queue warnings#13413Neenu1995 wants to merge 11 commits into
Conversation
… dynamic queue warnings
![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 connection property QueryExecutionThreadCount and an associated queryExecutor thread pool to handle background query execution. It also adds thread pool queue saturation monitoring with warning logs, configures pool threads as daemon threads, and includes comprehensive tests. The review feedback correctly points out a potential resource leak and sequential delay in the shutdown logic of BigQueryConnection when terminating the executors. If an interruption occurs during the termination of the metadata executor, the query executor's shutdown is skipped. A robust parallel shutdown approach with proper exception handling was suggested to resolve this issue.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated background query execution thread pool (queryExecutor) alongside the existing metadata fetch thread pool, including configurable thread counts, daemon thread configuration, and queue saturation monitoring with hysteresis warnings. The review feedback suggests optimizing the shutdown sequence in BigQueryConnection to avoid unnecessary blocking if an interrupt occurs, optimizing the warning flag reset in BigQueryJdbcMdc to prevent cache line bouncing, and using nested try-finally blocks in tests to ensure proper resource cleanup.
| */ | ||
| static ExecutorService newFixedThreadPool(int nThreads, ThreadFactory threadFactory) { | ||
| return new MdcThreadPoolExecutor( | ||
| nThreads, |
There was a problem hiding this comment.
We don't need coreThreads to match maxThreads imo; I think coreThreads being 3-4 would be sufficient.
| private final AtomicBoolean warningLogged = new AtomicBoolean(false); | ||
|
|
||
| private void monitorQueueSaturation(int queueSize) { | ||
| int corePoolSize = getCorePoolSize(); |
There was a problem hiding this comment.
I guess it needs to be maxPoolSize? Exceeding core is fine, new threads are being added.
2 participants
b/519201498
This PR implements connection-scoped thread pool isolation and dynamic saturation monitoring for the BigQuery JDBC driver.
queryExecutorandmetadataExecutorinstances per connection, properly cleaning them up on connection close.max(10, corePoolSize * 5)and resets only when it recovers belowmax(4, corePoolSize * 2).BigQueryJdbcMdcTestto inherit fromBigQueryJdbcLoggingBaseTest, and adds unit tests to verify lifecycle, saturation logging, and hysteresis behavior.