Skip to content

Conversation

@arvindKandpal-ksolves
Copy link
Contributor

@arvindKandpal-ksolves arvindKandpal-ksolves commented Dec 23, 2025

Why I'm doing:

Currently, third-party libraries that rely on java.util.logging (JUL), such as jprotobuf-rpc-socket, print logs directly to stdout (which redirects to fe.out) instead of the standard log files. This happens because the JUL-to-SLF4J bridge is not properly initialized, causing log leakage and making debugging difficult as these logs are missing from fe.log.

What I'm doing:

I have implemented the standard bridging mechanism to route java.util.logging calls to SLF4J (Log4j2).

Changes:

  1. Added the org.slf4j:jul-to-slf4j dependency in fe/pom.xml and fe/fe-core/pom.xml.
  2. Initialized LogManager.getLogManager().reset() and SLF4JBridgeHandler.install() in Log4jConfig.java to redirect logs.
  3. Added a unit test testJulToSlf4jBridge in Log4jConfigTest.java to verify that JUL logs are correctly captured by Log4j2.

This PR supersedes and fixes the unmerged PR #56156.

Fixes #31777

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc. (Logging behavior improvement)

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
  • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 4.0
    • 3.5
    • 3.4
    • 3.3

Note

Ensures logs from JUL-based libraries flow into the existing SLF4J/Log4j2 pipeline and match configured log levels.

  • Add org.slf4j:jul-to-slf4j dependency in fe/pom.xml and fe/fe-core/pom.xml
  • In Log4jConfig.reconfig(), install SLF4JBridgeHandler, remove root JUL handlers, and set JUL root level based on sys_log_level (DEBUGFINE, INFOINFO, WARNWARNING, ERROR|FATALSEVERE)
  • Add unit tests in Log4jConfigTest: testJulToSlf4jBridge (JUL logs captured by Log4j2) and testJulLevelMapping (JUL level mapping and invalid level handling)

Written by Cursor Bugbot for commit d1aaf5e. This will update automatically on new commits. Configure here.

@arvindKandpal-ksolves arvindKandpal-ksolves force-pushed the fix/31777-jul-to-slf4j-bridge branch from 9613e5f to 111649c Compare December 23, 2025 08:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a log leakage issue where third-party libraries using java.util.logging (JUL) were printing logs directly to stdout/fe.out instead of the standard log files. The solution implements the standard JUL-to-SLF4J bridging mechanism to route all JUL logs through Log4j2.

Key changes:

  • Added jul-to-slf4j dependency to enable JUL bridge functionality
  • Initialized the SLF4J bridge in the logging configuration to redirect JUL logs
  • Added a unit test to verify JUL logs are correctly captured by Log4j2

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
fe/pom.xml Added jul-to-slf4j dependency version 1.7.30 to parent POM dependency management
fe/fe-core/pom.xml Added jul-to-slf4j dependency reference in fe-core module
fe/fe-core/src/main/java/com/starrocks/common/Log4jConfig.java Added JUL bridge initialization by calling LogManager.reset() and SLF4JBridgeHandler.install() in reconfig()
fe/fe-core/src/test/java/com/starrocks/common/Log4jConfigTest.java Added unit test to verify JUL logs are captured and routed through Log4j2

@arvindKandpal-ksolves arvindKandpal-ksolves force-pushed the fix/31777-jul-to-slf4j-bridge branch from 9fe3940 to 741ed08 Compare December 23, 2025 13:55
@kevincai kevincai requested a review from Copilot December 23, 2025 13:55
kevincai
kevincai previously approved these changes Dec 23, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

@arvindKandpal-ksolves
Copy link
Contributor Author

@kevincai I have pushed the final implementation addressing all feedback.

1. Regarding Logic in reconfig() (Addressing Copilot)

I noticed Copilot suggested using LogManager.getLogManager().reset(). However, I intentionally decided against it.

  • Reason: reset() is a destructive operation that closes all handlers across the entire JVM logging context. This is risky as it could break logging for other third-party components (like Hadoop/Netty) running in the same process.
  • Solution: I used SLF4JBridgeHandler.removeHandlersForRootLogger(). This is a surgical approach that strictly prevents the stdout leakage (our main goal) without wiping out the global logging state.

2. Implementation Updates

  • Dynamic Level Alignment: JUL levels now sync with StarRocks (sys_log_level) to ensure performance.
  • Validation: Added strict validation using DEBUG_LEVELS.contains() logic (defaulting to INFO for unknown levels like TRACE).
  • Test Isolation: Updated testJulLevelMapping to verify the validation logic and added a try-finally block to strictly restore the original log levels.

3. CI Failure

I noticed the SQL-Tester failed with Base Compaction Failing and Manual compaction task is running. Since this PR only modifies Logging Configuration and has no impact on Backend storage/compaction logic, this appears to be a flaky test/environment issue.

Could you please trigger the CI checks again?

kevincai
kevincai previously approved these changes Dec 24, 2025
@kevincai
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Dec 24, 2025

rebase

✅ Branch has been successfully rebased

@kevincai kevincai force-pushed the fix/31777-jul-to-slf4j-bridge branch from 3ba311c to b65199c Compare December 24, 2025 02:22
@arvindKandpal-ksolves
Copy link
Contributor Author

@kevincai Thanks for the approval and the rebase!

I noticed the SQL-Tester failed with couldn't get JNIEnv (BE:10003) and Backend node not found.

Since this PR touches the Java logging initialization, could this be a side effect causing the JVM/BE to crash, or does this look like a known flaky CI issue to you?

Please let me know if you want me to revert anything.

@kevincai
Copy link
Contributor

@kevincai Thanks for the approval and the rebase!

I noticed the SQL-Tester failed with couldn't get JNIEnv (BE:10003) and Backend node not found.

Since this PR touches the Java logging initialization, could this be a side effect causing the JVM/BE to crash, or does this look like a known flaky CI issue to you?

Please let me know if you want me to revert anything.

It's an CI env issue, I will take care of it.

@alvin-celerdata
Copy link
Contributor

@cursor review

@arvindKandpal-ksolves arvindKandpal-ksolves force-pushed the fix/31777-jul-to-slf4j-bridge branch 2 times, most recently from 1cc51e1 to 2c22473 Compare December 26, 2025 05:35
@arvindKandpal-ksolves
Copy link
Contributor Author

@kevincai @alvin-celerdata The latest CI (FE UT) run failed on QueryPlannerTest (specifically testSqlDigestBlackList and others).

The error message <d != java.lang.String> indicates a weird environment/serialization issue, and QueryPlanner logic is completely unrelated to the logging configuration changes in this PR.

Could you please re-trigger the CI to clear this flake?

@alvin-celerdata
Copy link
Contributor

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no bugs!

@kevincai
Copy link
Contributor

@Mergifyio rebase

Fixes StarRocks#31777.

This PR bridges java.util.logging logs to slf4j so they are handled by log4j2
configuration instead of being leaked to stdout. This is necessary for
libraries like jprotobuf-rpc-socket which use java.util.logging.

Changes:
1. Add jul-to-slf4j dependency.
2. Initialize SLF4JBridgeHandler in Log4jConfig with dynamic level alignment.
3. Replace LogManager.reset() with removeHandlersForRootLogger() for safety.
4. Add unit test to verify the bridge.

Signed-off-by: arvindksi274-ksolves <[email protected]>
Fixes StarRocks#31777.

This PR bridges java.util.logging logs to slf4j so they are handled by log4j2
configuration instead of being leaked to stdout. This is necessary for
libraries like jprotobuf-rpc-socket which use java.util.logging.

Changes:
1. Add jul-to-slf4j dependency.
2. Initialize SLF4JBridgeHandler in Log4jConfig with dynamic level alignment.
3. Replace LogManager.reset() with removeHandlersForRootLogger() for safety.
4. Add validation logic to default to INFO for unsupported levels.
5. Add unit test to verify the bridge, level mapping, and fallback logic.

Signed-off-by: arvindksi274-ksolves <[email protected]>
@mergify
Copy link
Contributor

mergify bot commented Dec 27, 2025

rebase

✅ Branch has been successfully rebased

@kevincai kevincai force-pushed the fix/31777-jul-to-slf4j-bridge branch from 2c22473 to d1aaf5e Compare December 27, 2025 10:40
@sonarqubecloud
Copy link

@github-actions
Copy link

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

@github-actions
Copy link

[FE Incremental Coverage Report]

pass : 15 / 16 (93.75%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/common/Log4jConfig.java 15 16 93.75% [361]

@github-actions
Copy link

[BE Incremental Coverage Report]

pass : 0 / 0 (0%)

@kevincai kevincai enabled auto-merge (squash) December 27, 2025 11:50
@alvin-celerdata
Copy link
Contributor

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no bugs!

@kevincai kevincai merged commit 5b244bd into StarRocks:main Dec 27, 2025
64 checks passed
@github-actions
Copy link

@Mergifyio backport branch-4.0

@github-actions github-actions bot removed the 4.0 label Dec 27, 2025
@mergify
Copy link
Contributor

mergify bot commented Dec 27, 2025

backport branch-4.0

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Dec 27, 2025
wanpengfei-git pushed a commit that referenced this pull request Dec 27, 2025
…ackport #67129) (#67279)

Signed-off-by: arvindksi274-ksolves <[email protected]>
Co-authored-by: Arvind Kandpal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bridge java.util.logging to slf4j logging

3 participants