Skip to content

feat(bigquery-jdbc): support connection-scoped executor isolation and dynamic queue warnings#13413

Open
Neenu1995 wants to merge 5 commits into
mainfrom
per-conn-thread-pool-2
Open

feat(bigquery-jdbc): support connection-scoped executor isolation and dynamic queue warnings#13413
Neenu1995 wants to merge 5 commits into
mainfrom
per-conn-thread-pool-2

Conversation

@Neenu1995

@Neenu1995 Neenu1995 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

b/519201498

This PR implements connection-scoped thread pool isolation and dynamic saturation monitoring for the BigQuery JDBC driver.

  • Connection-Scoped Pools: Spawns independent queryExecutor and metadataExecutor instances per connection, properly cleaning them up on connection close.
  • Daemon Thread Factory: Configures the connection thread factory to mark all executor threads as daemon threads, preventing JVM shutdown hangs.
  • Hysteresis Saturation Warnings: Implements lightweight queue saturation logging that triggers dynamically when the queue reaches max(10, corePoolSize * 5) and resets only when it recovers below max(4, corePoolSize * 2).
  • Testing Improvements: Refactors BigQueryJdbcMdcTest to inherit from BigQueryJdbcLoggingBaseTest, and adds unit tests to verify lifecycle, saturation logging, and hysteresis behavior.

@Neenu1995 Neenu1995 requested review from a team as code owners June 10, 2026 17:14

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Neenu1995

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
this.openStatements.clear();

if (this.metadataExecutor != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update this closeImpl() to call all shutdown ops first & do await later, right now it awaits each shutdown individually.

We could force shutdownNow in Interrupted exception handler that already exists & we need to do it for all (including read/write clients), no need to handle them independently (I'd assume shutdownNow is no-op if was already successful)

Also, if Interrupted is thrown during bigQueryWriteClient.awaitTermination, openStatements is not properly cleaned up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants