Skip to content

Workarount to continue with log4j-2.17.0 by using slf4j18#11909

Closed
mcvsubbu wants to merge 3 commits intoapache:masterfrom
mcvsubbu:use-slf4j18
Closed

Workarount to continue with log4j-2.17.0 by using slf4j18#11909
mcvsubbu wants to merge 3 commits intoapache:masterfrom
mcvsubbu:use-slf4j18

Conversation

@mcvsubbu
Copy link
Copy Markdown
Contributor

As per suggestioin in PR 11903, attempting to use slf4j18 so that we can avoid bumping log4j version

As per suggestioin in PR 11903, attempting to use slf4j18
so that we can avoid bumping log4j version
@mcvsubbu
Copy link
Copy Markdown
Contributor Author

cc: @gortiz , @jackjlli

Copy link
Copy Markdown
Member

@jackjlli jackjlli left a comment

Choose a reason for hiding this comment

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

LGTM.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 30, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.63%. Comparing base (0d93791) to head (5f9054c).
⚠️ Report is 3012 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (0d93791) and HEAD (5f9054c). Click for more details.

HEAD has 24 uploads less than BASE
Flag BASE (0d93791) HEAD (5f9054c)
unittests 4 3
temurin 8 3
java-21 7 2
skip-bytebuffers-false 3 2
integration 4 0
integration1 2 0
skip-bytebuffers-true 3 1
custom-integration1 2 0
unittests2 2 0
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #11909       +/-   ##
=============================================
- Coverage     61.35%   46.63%   -14.72%     
+ Complexity     1147      940      -207     
=============================================
  Files          2376     1780      -596     
  Lines        128773    93307    -35466     
  Branches      19906    15078     -4828     
=============================================
- Hits          79007    43513    -35494     
- Misses        44039    46751     +2712     
+ Partials       5727     3043     -2684     
Flag Coverage Δ
custom-integration1 ?
integration ?
integration1 ?
java-11 46.57% <ø> (+18.93%) ⬆️
java-21 46.47% <ø> (-14.88%) ⬇️
skip-bytebuffers-false 46.59% <ø> (-14.68%) ⬇️
skip-bytebuffers-true 46.44% <ø> (-14.89%) ⬇️
temurin 46.63% <ø> (-14.72%) ⬇️
unittests 46.63% <ø> (-14.72%) ⬇️
unittests1 46.63% <ø> (+0.15%) ⬆️
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xiangfu0
Copy link
Copy Markdown
Contributor

Can you fix the tests?

Comment thread pom.xml Outdated
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>2.0.9</version>
<version>1.7.25</version>
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.

We cannot change slf4j, otherwise spart 3.5 will fail.

My suggestion was to try to use slf4j2 v2.0.x and log4j-slf4j18-impl as adaptor, although I'm not sure if that would work

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that is what i had tried in my first commit, but got the error:

java.lang.abstractmethoderror: receiver class org.apache.logging.slf4j.slf4jserviceprovider does not define or inherit an implementation of the resolved method 'abstract java.lang.string getrequestedapiversion()' of interface org.slf4j.spi.slf4jserviceprovider.

presumably related to https://issues.apache.org/jira/browse/LOG4J2-3370

@gortiz
Copy link
Copy Markdown
Contributor

gortiz commented Nov 3, 2023

Let me summarize the current state:

  • We wanted to be able to execute Pinot in Java 21.
  • Spark 3.2 does not support Java 21, so I updated it to Spark 3.5
  • Spark 3.5 does uses slf4j v2.0.
  • Log4j2 v2.17.1 does not support slf4j v2.0.
  • I updated log4j2 to v2.20.0, which supports slf4j v2.0
  • LinkedIn reports they have seen that newer versions of log4j2 have some thread contention
  • @mcvsubbu tried to revert log4j to 2.17.x family without success due to runtime errors in Spark 3.x Pinot plugin.

I don't think there is a way to keep Spark 3.5 (and therefore Java 21 support) without upgrading log4j2. What I've asked in another PR is whether this thread contention problem has been reported to log4j2 maintainers. I've made a fast search and didn't find issues related to that in their repository.

I think the best solution would be to solve the thread contention issue in log4j2. We don't need to actually fix it our self, but it would be great if you could report the issue in their repo so they know it and therefore may fix it. As said in the other thread, we don't want to keep using older versions of compatible libraries due to perf regressions. That is a dangerous path that can end up having a vulnerable dependency we cannot upgrade.

@Jackie-Jiang
Copy link
Copy Markdown
Contributor

Discussed offline, and here is the update:

  • LinkedIn figured out a way to ping the old log4j version
  • The thread contention issue is fixed in newer log4j2 version, and Pinot project is already upgraded with the latest one

Closing this thread

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.

6 participants