Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors BigQueryJdbcCustomLogger usage across several classes to be static final rather than instance-specific, preventing memory leaks caused by excessive child logger registrations. It also adds unit tests to verify that statement and metadata creation do not leak loggers, and updates the Maven configuration to permit reflective access to JDK-internal logger fields during testing. Feedback on the changes suggests avoiding the use of a hardcoded relative path (./logs) for test logging to prevent workspace pollution, recommending the system's temporary directory instead.
Contributor
Author
|
/gcprun |
logachev
approved these changes
Oct 21, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
b/576218509
LOG = new BigQueryJdbcCustomLogger(this.toString())field with one
static finallogger per class inBigQueryStatement,BigQueryConnection,BigQueryDatabaseMetaData,BigQueryCallableStatement,BigQueryPooledConnection(+BigQueryPooledConnectionWrapper),PooledConnectionListener, andDataSource. Each logger is named after itsown class so the
CLASScolumn fallback in the formatter stays correct.setParent(...)call in the 1-argBigQueryJdbcCustomLoggerconstructor; the 2-arg constructor it delegates toalready sets the parent. The second call forced a full scan of the parent's
child list under the global
treeLockon every construction.BigQueryJdbcCustomLoggerTest:testLoggerIsParentedToDriverRootLoggerlocks in that every logger isstill parented to the driver root logger (handlers/levels keep flowing).
testCreatingStatementsDoesNotRegisterChildLoggersandtestCreatingDatabaseMetaDataDoesNotRegisterChildLoggersconstruct 1,000objects and assert the root logger's child count does not grow. Both fail
on
mainand pass with this change.--add-opens=java.logging/java.util.logging=ALL-UNNAMEDto the surefireargLine(test-only) so the regression tests can read the JDK-internalchild-logger list on JDK 17+.
No behavior change to per-connection log files or formatting: routing and the
[CONNECTION_ID]column come from the MDC / record parameters, not from thelogger name.