feat(bigquery-jdbc): migrate statement execution thread tracking to connection-scoped executor#13454
feat(bigquery-jdbc): migrate statement execution thread tracking to connection-scoped executor#13454Neenu1995 wants to merge 7 commits into
Conversation
…onnection-scoped executor
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous background task management in the BigQuery JDBC driver by replacing raw Thread references with Future<?> objects. This allows tasks to be managed and cancelled via the connection's ExecutorService rather than spawning unmanaged threads. The review feedback highlights a critical issue in the temporary wrapThread compatibility bridge in BigQueryDatabaseMetaData.java, where the anonymous Future implementation violates the standard Future contract regarding cancellation and completion states, which could lead to incorrect state tracking during resource cleanup.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous execution model of the BigQuery JDBC driver by replacing raw Thread objects with Future<?> tasks managed by a connection-scoped ExecutorService. Feedback focuses on a potential deadlock risk when executing interdependent tasks on a limited executor, contract violations in the temporary wrapThread Future adapter (specifically around isDone, cancel, and swallowing InterruptedException), and opportunities to simplify redundant Future status checks before cancellation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the JDBC driver to manage background tasks using Future objects and a bounded cached thread pool instead of raw threads, preventing potential OutOfMemoryErrors under heavy load. Feedback focuses on fixing the Future contract implementation in the temporary wrapThread bridge to ensure it blocks correctly for unstarted threads, and ensuring that already-submitted tasks are cancelled if a thread allocation failure occurs during JSON result set processing.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors thread management in the BigQuery JDBC driver by replacing raw Thread usage with Future<?> tasks and transitioning to a connection-scoped bounded cached thread pool to prevent unbounded thread creation and OOMs. Feedback focuses on improving the temporary wrapThread compatibility bridge in BigQueryDatabaseMetaData to avoid busy-spinning when a thread is in the NEW state and to prevent race conditions in cancel(). Additionally, the exception handling in BigQueryStatement.processJsonResultSet should be broadened to catch all exceptions to prevent thread leaks and avoid swallowing InterruptedException.
b/519201498
This PR migrates the asynchronous processing tasks of the JDBC driver away from the legacy static global thread pool (
queryTaskExecutor) to the modern, connection-scopedExecutorService(connection.getExecutorService()).Key Changes
Statement & Stream Refactoring:
queryTaskExecutorfromBigQueryStatement.populateArrowBufferedQueueto submit streaming and buffering tasks to the connection-scoped executor pool.BigQueryStatementandBigQueryArrowResultSetfrom rawThreadreferences to modern, cancelableFuture<?>task handles.Isolated Backward-Compatibility Wrapper:
BigQueryDatabaseMetaData(which is still pending future refactoring to the executor pool).wrapThread(Thread)helper method insideBigQueryDatabaseMetaDatato wrap raw background threads into cancelableFuture<?>adapters on the fly before passing them to the result set.Futurewrapper state tracking to strictly satisfy theFuturespecification contract (handlingcancel,isCancelled, andisDonecorrectly using a volatile boolean flag to prevent duplicate cancellation triggers).