Skip to content

Fix logging when an OFF connection initializes first - #1670

Merged
cathleeny merged 4 commits into
mainfrom
SC-241918-logging-off-to-enabled
Sep 8, 2026
Merged

Fix logging when an OFF connection initializes first#1670
cathleeny merged 4 commits into
mainfrom
SC-241918-logging-off-to-enabled

Conversation

@cathleeny

@cathleeny cathleeny commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Description

Fix process-wide JUL initialization when the first connection uses LogLevel=OFF.

  • OFF suppresses the shared parent logger without creating a handler or permanently completing initialization.
  • The first logging-enabled connection installs the single shared handler.
  • Once enabled, later connections—including OFF connections—do not reconfigure the logger.
  • Failed handler creation remains retryable.

Testing

  • Unit tests cover OFF -> TRACE, TRACE -> OFF, concurrent initialization without duplicate handlers, and retry after failed handler creation.
  • JulLoggerTest and LoggingUtilTest: 30 tests passed.
  • Thin and uber JARs were manually verified: OFF -> TRACE creates one handler, while TRACE -> OFF retains the original handler and level.

Additional Notes to the Reviewer

This is the short-term fix and intentionally keeps handler creation inside JulLogger.initLogger().

Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No issues identified by the review bot.

handler.setLevel(level);
handler.setFormatter(new Slf4jFormatter());
jdbcJulLogger.addHandler(handler);
if (isLoggerInitialized) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the issue will still remain, logger will remain disabled if first connection had it disabled

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I verified the OFF to enabled change. The OFF path returns without setting isLoggerInitialized, so the later enabled call installs a handler, updates the logger level, and marks initialization complete. testInitLoggerPromotesFromOffToEnabled covers this, and I also tested locally

Comment thread src/main/java/com/databricks/jdbc/log/JulLogger.java Outdated
// parent package logger. Using "com.databricks" as the prefix captures all JDBC driver
// classes as well as shaded dependencies (SDK, Apache HTTP client, etc.)
Logger jdbcJulLogger = Logger.getLogger(PARENT_CLASS_PREFIX);
jdbcJulLogger.setUseParentHandlers(false);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we want to set this on OFF only path? This can be done after checking OFF level

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The OFF path needs it to prevent logs from propagating to root handlers, so both the OFF path and enabled path need this

Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Review bot failed — see workflow logs.

Reason: model did not call finalize_review and the final text was not parseable JSON: Empty response

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Review bot failed — see workflow logs.

Reason: model did not call finalize_review and the final text was not parseable JSON: Empty response

@cathleeny
cathleeny enabled auto-merge September 8, 2026 16:10

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No issues identified by the review bot.

@cathleeny
cathleeny added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 7dd9082 Sep 8, 2026
23 of 24 checks passed
@cathleeny
cathleeny deleted the SC-241918-logging-off-to-enabled branch September 8, 2026 16:57
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