refactor(bigquery-jdbc): migrate metadata thread management to connection-scoped executor#13556
refactor(bigquery-jdbc): migrate metadata thread management to connection-scoped executor#13556keshavdandeva wants to merge 3 commits into
Conversation
…tion-scoped executor
There was a problem hiding this comment.
Code Review
This pull request refactors BigQueryDatabaseMetaData to use a shared, lazily-initialized metadataExecutor from BigQueryConnection instead of creating and shutting down local thread pools for metadata operations. Additionally, it simplifies argument and parameter processing by executing them sequentially rather than in parallel, and removes the temporary wrapThread utility. Feedback is provided regarding a potential NullPointerException during connection teardown, as the lazily-initialized metadataExecutor may remain null if no metadata queries are executed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors metadata fetching in the BigQuery JDBC driver by lazily initializing the metadataExecutor in BigQueryConnection and utilizing it across various metadata operations in BigQueryDatabaseMetaData instead of spawning raw threads or local thread pools. Additionally, several processing tasks are refactored to run sequentially, and the temporary wrapThread utility is removed. The review comments identify two critical issues: first, a potential pool-induced deadlock (thread starvation) caused by submitting both parent fetcher tasks and child API tasks to the same limited fixed thread pool; second, a potential NullPointerException during connection closure because the lazily initialized metadataExecutor may remain null.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors metadata fetching in BigQueryDatabaseMetaData to utilize shared executors from BigQueryConnection instead of spawning local thread pools, and transitions several parallel tasks to sequential execution. The temporary wrapThread utility has also been removed. The review feedback points out critical concurrency and lifecycle issues: the lazy initialization of metadataExecutor in BigQueryConnection requires a null check in close() to prevent a NullPointerException and synchronization to avoid resource leaks. Furthermore, in BigQueryDatabaseMetaData, the reviewer suggests moving the cancellation of background futures to finally blocks to ensure they are always cancelled when the fetcher thread exits, while also properly restoring the thread's interrupted status.
b/520400589
This PR optimizes thread management for JDBC metadata methods, implementing a deadlock-free, bipartite thread pool design and unifying resource lifecycle management.
Key Changes
Bipartite Thread Pool Architecture (Deadlock Prevention):
connection.getExecutorService()). Because the cached pool scales dynamically as needed, this structurally prevents pool-induced deadlocks (thread starvation).connection.getMetadataExecutor()). This bounds parallel network traffic to prevent overwhelming the BigQuery backend.metadataFetchThreadCountis configured to1.Connection-Scoped Shared Executor:
getSchemas,getTables,getColumns,getProcedures,getProcedureColumns,getFunctions, andgetFunctionColumns) from spawning raw threads or short-lived local pools to connection-managed executors.Lower CPU Overhead (Sequential Row-Building):
routineProcessorExecutor,processArgsExecutor,tableProcessorExecutor, andprocessParamsExecutor) previously used for row-building.Teardown of Legacy Bridges:
wrapThread(Thread)compatibility adapter inBigQueryDatabaseMetaData.java.testWrapThread_*cases fromBigQueryDatabaseMetaDataTest.java. Metadata queries now track and cancel background tasks natively via standardFuture<?>handles.Test Suite Refactoring:
BigQueryDatabaseMetaDataTestsetup to inject real, connection-scoped cached executor services, ensuring that background metadata tasks execute deterministically during testing.@AfterEachteardown logic to prevent test-suite thread leaks.