solr icon indicating copy to clipboard operation
solr copied to clipboard

Published 20 hours ago • verified publisher icon apache

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

Open szhou1998 opened this issue 1 year ago • 6 comments

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

Description

https://github.com/apache/solr/pull/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.

Checklist

Please review the following and check all that apply:

  • [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • [x] I have created a Jira issue and added the issue ID to my pull request title.
  • [x] 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)
  • [x] 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

szhou1998 avatar Sep 10 '24 00:09 szhou1998

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!

epugh avatar Sep 10 '24 11:09 epugh

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.

szhou1998 avatar Sep 10 '24 20:09 szhou1998

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

epugh avatar Sep 10 '24 21:09 epugh

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

szhou1998 avatar Sep 10 '24 22:09 szhou1998

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 avatar Sep 11 '24 15:09 epugh

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!

epugh avatar Sep 12 '24 10:09 epugh

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

dsmiley avatar Oct 29 '24 20:10 dsmiley