Uh oh!
There was an error while loading. Please reload this page.
fix(bigquery-jdbc): feature branch failing tests - #13287
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates OpenTelemetry tracing and logging into the BigQuery JDBC driver, adding a custom JUL logging handler (OpenTelemetryJulHandler) and updating core connection, statement, and metadata classes to support tracing and context propagation. The review feedback identifies a performance risk from synchronous blocking network calls inside the logging handler's publish method, recommending offloading to a bounded thread pool. Additionally, it highlights a thread-safety issue in ensureGlobalHandlerAttached that could lead to duplicate handlers, suggesting the use of an explicit lock to secure concurrent initialization.
I am having trouble creating individual review comments. Click here to see my feedback.
java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/OpenTelemetryJulHandler.java (110)
Calling loggingClient.write synchronously inside the publish method of a logging Handler introduces a blocking network call on the application's critical path. This can severely degrade performance, especially when logging is verbose (e.g., at FINE or INFO levels). Consider offloading the log writing to a background bounded thread pool (e.g., ThreadPoolExecutor with a defined maximum size) or queue, or configuring the Logging client with asynchronous batching options.
References
- For safety, use a bounded thread pool (e.g.,
ThreadPoolExecutorwith a defined maximum size) instead of an unbounded one (e.g.,Executors.newCachedThreadPool()), even if the current logic seems to limit concurrent tasks.
java-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcOpenTelemetry.java (143-155)
The ensureGlobalHandlerAttached method is not thread-safe. If multiple connections are initialized concurrently, multiple threads might check logger.getHandlers() simultaneously, find no handler, and both add a new OpenTelemetryJulHandler. This would result in duplicate handlers and duplicate log exports. To protect this shared state while ensuring thread safety and visibility in performance-sensitive code, prefer using an explicit lock over the synchronized keyword.
privatestaticfinaljava.util.concurrent.locks.ReentrantLocklock = newjava.util.concurrent.locks.ReentrantLock();
publicstaticvoidensureGlobalHandlerAttached() {
lock.lock();
try {
Loggerlogger = Logger.getLogger(BIGQUERY_NAMESPACE);
booleanpresent = false;
for (Handlerh : logger.getHandlers()) {
if (hinstanceofOpenTelemetryJulHandler) {
present = true;
break;
}
}
if (!present) {
logger.addHandler(newOpenTelemetryJulHandler());
}
} finally {
lock.unlock();
}
}References
- In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
b/517498094
This PR fixes dependency analysis failures and flaky test issues identified in the OpenTelemetry integration feature branch.