feat(bqjdbc): Per connection logs - Add PerConnectionFileHandler#12899
feat(bqjdbc): Per connection logs - Add PerConnectionFileHandler#12899
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 via a new PerConnectionFileHandler and restricts the visibility of BigQueryJdbcMdc members. The review feedback focuses on preventing resource leaks by managing the lifecycle of internal FileHandler instances and improving robustness through null checks, exception-safe resource closure, and the use of ErrorManager for internal error reporting.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| handlers.computeIfAbsent( | ||
| connectionId, | ||
| id -> { | ||
| try { |
There was a problem hiding this comment.
Can you move it to a helper function instead of lambda? (And can be merged with setup for defaultHandler)
| }); | ||
| } | ||
|
|
||
| if (handler != null) { |
| } | ||
|
|
||
| public void closeHandler(String connectionId) { | ||
| if (connectionId != null) { |
There was a problem hiding this comment.
nit: I think this check is not required. If it is null, .remove() is going to return null which is handled.
| } | ||
|
|
||
| private String getLogFilePath(String id) { | ||
| String path = baseLogPath; |
There was a problem hiding this comment.
Can we store baseLogPath as a Path (maybe using toAbsolutePath when initialized and use `baseLogPath.resolve("BigQuery-" + id + ".log")?
2 participants
Uh oh!
There was an error while loading. Please reload this page.