Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-16976: Remove log4j-jul jar and use slf4j bridge for JUL #2703

Merged
merged 11 commits into from
Oct 30, 2024

Conversation

szhou1998
Copy link
Contributor

@szhou1998 szhou1998 commented Sep 10, 2024

https://issues.apache.org/jira/browse/SOLR-16976

Description

#1765 introduced a change that causes an exception when remote JMX is enabled (stack trace included in Jira description).

Solution

@elyograg provided a patch which resolves this issue by removing log4j-jul and using the slf4j bridge for JUL. I removed the NOCOMMIT from the patch.

Tests

@elyograg tested the patch. Added a test that there's no exception on Solr startup with remote JMX enabled.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@epugh
Copy link
Contributor

epugh commented Sep 10, 2024

I don't have much knowledge in how logging works. One thing that would help, would you mind creating a BATS style test that demonstrates the bug and the fix? I believe you could write a test that starts up solr with remote JMX, and then we see the bug, and then the fix. The bats tests are in ./solr/packaging/test. Let me know if you have any questions!

@szhou1998
Copy link
Contributor Author

I don't have much knowledge in how logging works. One thing that would help, would you mind creating a BATS style test that demonstrates the bug and the fix? I believe you could write a test that starts up solr with remote JMX, and then we see the bug, and then the fix. The bats tests are in ./solr/packaging/test. Let me know if you have any questions!

I've removed the patch and added a test to start solr with remote JMX enabled. It should fail with the exception seen in the Jira.

@epugh
Copy link
Contributor

epugh commented Sep 10, 2024

Humm... I ran the "Solr Script Tests" expecting it to fail... I can run it locally tomorrow and see what I see!

@szhou1998
Copy link
Contributor Author

Ah my mistake, Solr starts even though the exception is logged. Added a refute_output for the exception.

@epugh
Copy link
Contributor

epugh commented Sep 11, 2024

okay, the refute wasn't actually matching.. i found that we need to consult the log file. Also, I was getting a RMI Port exception.. Can you validate that this test fails for you, and hten add in your patch?

@epugh
Copy link
Contributor

epugh commented Sep 12, 2024

Okay, so, after putting in the slf4j bridge, I see that the bats tests is now failing (and thats a good thing) because it means we don't see the error:

not ok 85 SOLR-16976 solr starts with remote JMX enabled # in 7641 ms
# (from function `assert_output' in file /home/runner/work/solr/solr/.gradle/node/packaging/node_modules/bats-assert/src/assert.bash, line 247,
#  in test file test/test_start_solr.bats, line 40)
#   `assert_output --partial 'java.lang.ClassNotFoundException: org.apache.logging.log4j.jul.LogManager'' failed

@szhou1998 would you mind flipping the assert_output to a refute_output and lets see if the test now passes!

@szhou1998 szhou1998 force-pushed the SOLR-16976 branch 2 times, most recently from 2993c1c to 3ba7b29 Compare September 12, 2024 20:40
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

There's probably a .sha1 and maybe more to remove from the licenses/ dir.

Related to my comment on the JIRA issue, might you propose a short summary for CHANGES.txt? Like... this is maybe a bug or improvement; seems more like the latter but I defer to your judgement. We removed a dependency we didn't need (yay). We initialized JUL logging and thus... ?

Also looking for how to refer to you for attribution.

solr assert --started http://localhost:${SOLR_PORT} --timeout 5000

run cat ${SOLR_LOGS_DIR}/solr-${SOLR_PORT}-console.log
refute_output --partial 'java.lang.ClassNotFoundException: org.apache.logging.log4j.jul.LogManager'
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets generalize this to simply "Exception" -- we don't want any exceptions printed for whatever reason (JUL or otherwise).

@szhou1998 szhou1998 force-pushed the SOLR-16976 branch 2 times, most recently from ea8abe5 to 9e5c316 Compare October 29, 2024 19:17
@dsmiley
Copy link
Contributor

dsmiley commented Oct 29, 2024

Please stop force-pushing to PRs -- it resets the review state in GitHub.
Any way, I will merge this soon; thank you!

@dsmiley dsmiley merged commit dbf1c50 into apache:main Oct 30, 2024
5 checks passed
dsmiley pushed a commit that referenced this pull request Oct 30, 2024
Remove log4j-jul jar and use slf4j bridge for JUL to prevent exception from being logged when remote JMX
  is enabled .
Updated jetty.xml to initialize the bridge super early.

Co-authored-by: Eric Pugh <[email protected]>
Co-authored-by: David Smiley <[email protected]>

(cherry picked from commit dbf1c50)
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.

4 participants