test(bigquery-jdbc): fix minor issues and run integration tests for otel#13866
test(bigquery-jdbc): fix minor issues and run integration tests for otel#13866keshavdandeva wants to merge 5 commits into
Conversation
![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 integrates OpenTelemetry tracing and logging into the BigQuery JDBC driver, introducing telemetry configuration management, context propagation across asynchronous boundaries, and GCP exporters. Key feedback highlights critical issues: a potential resource and thread leak from unclosed cached OpenTelemetrySdk instances, a violation of the java.util.Properties contract by passing a non-String object that could cause ClassCastExceptions in other drivers, a missing check for the private key path property during credential resolution, and classloader leaks in application servers caused by registering a JVM shutdown hook in a static initializer.
keshavdandeva
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces reference counting for OpenTelemetry SDK instances in the BigQuery JDBC driver to ensure proper resource cleanup when connections are closed. It adds a releaseSdk method, replaces the global shutdown hook with reference-tracked caching using a new CachedSdk class, and updates BigQueryConnection to release its SDK on close. In the test suite, polling retry limits and delays are increased to improve stability. The review feedback correctly identifies a potential race condition in releaseSdk where a newly cached SDK could have its reference count incorrectly decremented and closed prematurely; a check should be added inside computeIfPresent to verify the SDK instance before decrementing.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces reference counting for cached OpenTelemetry SDK instances in BigQueryJdbcOpenTelemetry to properly manage their lifecycle and close them when no longer needed, while also updating credential resolution logic in BigQueryConnection and adjusting integration test polling parameters. The review feedback suggests several key improvements: avoiding blocking sdk.close() calls inside ConcurrentHashMap compute lambdas to prevent locking issues, setting the connection's openTelemetry reference to null after release to prevent double-closing bugs, and initializing the SDK reference count to 1 by default to simplify instantiation.
1 participant
b/537284101
This PR fixes multiple OpenTelemetry resource leaks.
Key Changes
OpenTelemetrySdkin aCachedSdkto track usage. The SDK and its background threads are now explicitly closed when the last JDBCConnectionusing them is closed.Runtime.addShutdownHookinBigQueryJdbcOpenTelemetry. This prevents severeMetaspacememory leaks when the driver is undeployed in Application Servers (like Tomcat/JBoss).resolveEffectiveCredentials()to properly detectOAUTH_PVT_KEY_PATH_PROPERTY_NAME(key file paths), preventing erroneous fallbacks to Application Default Credentials.ITOpenTelemetryTestfrom 500ms to 3000ms. This prevents the GCP Logging API from rejecting the test withRESOURCE_EXHAUSTED(Status 429) errors due to exceeding the 60 requests/minute quota limit.