Issue 2837: Remove unnecessary log4j bindings from Bookkeeper packages

[RaulGracia]

Motivation

Remove log bindings from bookkeeper-common and bookkeeper-server.

Changes

• Removed log4j bindings from bookkeeper-common and bookkeeper-server main code:
  • In both packages, these bindings are only used in test scope, as there are tests that directly rely on log4j APIs for their test logic.
  • To allow bookkeeper-server to work only with slf4j, we needed to remove direct use of log4j API in FileSystemUpgrade class.

Master Issue: #2837

[zymap]

@RaulGracia Any updates on this?

[RaulGracia]

Thanks @zymap, I sent out this PR out for getting feedback related to this comment: https://github.com/apache/bookkeeper/pull/2816/files#r729987757. It is in draft mode to prevent it from getting merged before we agree that it addresses the original comment. Now I see that there is a conflict from a recent merge, I will resolve it.
cc/ @pkumar-singh @eolivelli

[pkumar-singh]

@RaulGracia Looks good.

[zymap]

Tests failure:

org.apache.bookkeeper.tests.integration.stream.BkCtlTest

  Test simpleTest FAILED (2.6s)

  java.lang.AssertionError
      at org.apache.bookkeeper.tests.integration.stream.BkCtlTest.simpleTest(BkCtlTest.java:84)

  Test putGetKey PASSED (5.9s)
  Test createTable PASSED (2.2s)
  Test createNamespace PASSED (2.3s)
  Test listBookies FAILED

  java.lang.AssertionError
      at org.apache.bookkeeper.tests.integration.stream.BkCtlTest.listBookies(BkCtlTest.java:51)

  Test incGetKey PASSED (6.2s)
  Test showLastMark FAILED

  java.lang.AssertionError
      at org.apache.bookkeeper.tests.integration.stream.BkCtlTest.showLastMark(BkCtlTest.java:66)

@RaulGracia Could you please take a look at the failure tests? I rerun the tests and it still failed 😕

[RaulGracia]

Thanks @zymap, I think that the problem may be as follows:

• The comments in the original PR Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 #2816 suggested to remove Log4j bindings from bookkeeper-common and bookkeeper-server (at least from the main code).
• I have been able to do that for bookkeeper-common.
• I have done the same for bookkeeper-server. However, it looks like this module was importing (even before PR Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 #2816) some of these binding dependencies, given that when I remove them, integration tests fail. The failures are related to some tests that relay on the log output to validate bkctl commands.

I will work on finding the best way to make bkctl to have the necessary log4j dependencies, while removing them from bookkeeper-server, as this was the original comment. My only concern is whether or not there could be more modules relying on the log4j bindings in the same way as bkctl tests do.

[RaulGracia]

@eolivelli @pkumar-singh @zymap I think that this PR now addresses the original comment that suggested that we should not be bringing log implementation bindings in neither bookkeeper-server and bookkeeper-common. Note that for bookkeeper-common this was true, but I think that was not the case for bookkeeper-server (as visible, FileSystemUpgrade class was using log4j APIs within bookkeeper-server main code). I have set this PR ready to review, so it would be great to get your feedback about it.

Also, this PR has been tagged as version 4.14.3, but I think that this is not correct; This PR is related to PR #2816 that targets 4.15 (current master).

[pkumar-singh]

Looks very good.

[pkumar-singh]

Looks good to me. Approved. Thanks -Prashant.

…

On Mon, Oct 25, 2021 at 10:52 AM Raúl Gracia ***@***.***> wrote: @eolivelli <https://github.com/eolivelli> @pkumar-singh <https://github.com/pkumar-singh> @zymap <https://github.com/zymap> I think that this PR now addresses the original comment <#2816 (comment)> that suggested that we should not be bringing log implementation bindings in neither bookkeeper-server and bookkeeper-common. Note that for bookkeeper-common this was true, but I think that was not the case for bookkeeper-server (as visible, FileSystemUpgrade class was using log4j APIs within bookkeeper-server main code). I have set this PR ready to review, so it would be great to get your feedback about it. Also, this PR has been tagged as version 4.14.3, but I think that this is not correct; This PR is related to PR #2816 <#2816> that targets 4.15 (current master). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2838 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APQ5IN4KNKOFRE633ZADN53UIWKMTANCNFSM5GIT2YDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[nicoloboschi]

LGTM

[eolivelli]

Overall lgtm

I left one comment PTAL

[eolivelli]

LGTM

[eolivelli]

I am not sure that this change fits 4.14.x, because we are changing the dependencies, it is kind of breaking change.

WDTY ?

[RaulGracia]

@eolivelli you are right and I posted the same here: #2838 (comment)
This is a change related to another PR that is only in master, so it should not go to branch 4.14.

[zymap]

So should I remove the #2816 from 4.14.x as well? Because this PR looks like an improvement of PR #2816. @eolivelli @RaulGracia

[eolivelli]

@zymap yes, I would remove this work (#2816) from branch-4.14

[RaulGracia]

@zymap, @eolivelli is right, PR #2816 focuses on master only, as well as this PR.

[eolivelli]

@zymap have you reviewed this PR ? can we merge to master in your opinion ?

[zymap]

LGTM

[eolivelli]

committed to master branch, thank you @RaulGracia