Skip to content

fix(bigquery-jdbc): stop per-ResultSet logger registration leak - #14718

Open
Neenu1995 wants to merge 3 commits into
neenu/fix-logger-leakfrom
neenu/fix-rslogger-leak
Open

Neenu1995 wants to merge 3 commits into
neenu/fix-logger-leakfrom
neenu/fix-rslogger-leak

Conversation

@Neenu1995

@Neenu1995 Neenu1995 commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

b/576218509

One registered BigQueryJdbcCustomLogger per class, shared; the facade holds that delegate plus the connection ID and stamps the ID into LogRecord params[0] exactly as before, so per-connection file routing for un-proxied ResultSet calls is unchanged. Hot-path trace methods still set the source class/method explicitly and allocate nothing
when the level is disabled.

@Neenu1995
Neenu1995 requested review from a team as code owners October 21, 2026 17:01

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses a logger registration leak in the BigQuery JDBC driver by refactoring BigQueryJdbcResultSetLogger into a final facade class that delegates to shared, class-level loggers, and by converting instance loggers in various JDBC classes to static fields. Additionally, unit tests are added to verify that creating statements, metadata, and result-set loggers does not leak child loggers. The review feedback suggests declaring the lazily-initialized logger fields in BigQueryJdbcResultSetLogger as volatile to ensure thread safety across shared global facades, and modifying the warning method to avoid calling String.format when no arguments are provided, preventing potential UnknownFormatConversionException errors.

@Neenu1995
Neenu1995 changed the base branch from main to neenu/fix-logger-leak October 21, 2026 17:28
Neenu1995 and others added 2 commits October 21, 2026 13:29
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
*/
/** Cold path: builds a record whose caller is inferred lazily by the formatter. */
private void logWithCaller(Level level, Throwable thrown, String msg) {
if (!delegate.isLoggable(level)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we override isLoggable to do that check rather than always do delegate.isLoggable?

logTrace(Level.FINE, "unknown", msgSupplier);
}

void warning(String format, Object... args) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants