feat(bqjdbc): Integrate the PerConnectionFileHandler with BigQueryJdbcRootLogger#12933
feat(bqjdbc): Integrate the PerConnectionFileHandler with BigQueryJdbcRootLogger#12933
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 implements connection-specific logging by introducing a unique connectionId and utilizing Mapped Diagnostic Context (MDC) across BigQueryConnection and BigQueryStatement. Key changes include wrapping JDBC methods in try-finally blocks to manage MDC state and updating the logging formatter to include connection identifiers. Review feedback highlights potential NullPointerException vulnerabilities in BigQueryStatement when accessing the connection object after it has been closed, as well as a suggestion to refine MDC cleanup to prevent clearing external application context.
| // Example: 2026-04-22 10:16:00.123 INFO 12345 --- [main ] | ||
| // com.google.cloud.bigquery.jdbc.BigQueryConnection connect : Connection | ||
| // successful | ||
| // Expected log format: yyyy-MM-dd HH:mm:ss.SSS [CONNECTION_ID] LEVEL PID --- [THREAD] CLASS |
There was a problem hiding this comment.
Why do we need Connection_ID? Isn't it 1 file per connection now?
| addOpenStatements(currentStatement); | ||
| return currentStatement; | ||
| } finally { | ||
| BigQueryJdbcMdc.clear(); |
There was a problem hiding this comment.
Can we make registerInstance() to return AutoCloseable() object so we don't need to call clear() every time?
| @@ -706,102 +782,125 @@ public void clearWarnings() { | |||
|
|
|||
| @Override | |||
| public boolean getAutoCommit() { | |||
| checkClosed(); | |||
| return this.autoCommit; | |||
| try { | |||
There was a problem hiding this comment.
I'd suggest using the same approach from Telemetry.
Moving real logic to the private Impl() so it is easier to distinguish business logic vs logging setup\tracer setup etc.
Now multiple places have try/catch/finally blocks inside another try\catch\finally and that's difficult to read.
2 participants
Uh oh!
There was an error while loading. Please reload this page.