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.

Sign up for GitHub

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

Jump to bottom

MINOR: log4j2 upgrade follow-up #18290

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

MINOR: log4j2 upgrade follow-up #18290

wants to merge 1 commit into from

Conversation

frankvicky
Copy link
Contributor

Please refer to #17373 (comment) for further details.
This PR does the following things:

  • Move deprecation warning to kafka-run-class
  • Remove unnecessary jacksonDatabindYaml dependency from modules
  • Add upgrade notes about log4j2.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions bot added triage PRs from the community build Gradle build or GitHub Actions small Small PRs labels Dec 21, 2024
@chia7712
Copy link
Member

@frankvicky as @ijuma's comment in https://issues.apache.org/jira/browse/KAFKA-18247, please add the log4j2 transform ci to the upgrade note.

ijuma
ijuma reviewed Dec 21, 2024
View reviewed changes
docs/upgrade.html
@@ -227,6 +227,11 @@ <h5><a id="upgrade_400_notable" href="#upgrade_400_notable">Notable changes in 4
Java 8 support has been removed in Apache Kafka 4.0
See <a href="https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=181308223">KIP-750</a> for more details
</li>
<li>
Logging framework has been migrated from Log4j to Log4j2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we mention the behavior if you do not convert the configuration files? I believe there is a compatibility mode with some limitations?

Copy link
Member

Choose a reason for hiding this comment

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

Log4j provides a way to convert Log4j 1.x properties at runtime (Log4j Migration Guide).

To enable this feature, we can set both LOG4J_COMPATIBILITY=true and specify the configuration file using LOG4J_CONFIGURATION_FILE=file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to automatically detect if log4j 1.x properties are used and, if so, automatically enable this mode? It seems like a huge problem if customers lose their logging while upgrading to 4.0+.

Copy link
Member

Choose a reason for hiding this comment

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

We can always enable that compatibility feature as the bridge jar is already included now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better if it can be always enabled with no significant downside.

@github-actions github-actions bot removed the triage PRs from the community label Dec 22, 2024
chia7712
chia7712 reviewed Dec 22, 2024
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@frankvicky thanks for your patch

build.gradle
@@ -1588,7 +1588,6 @@ project(':test-common') {
implementation project(':storage')
implementation project(':server-common')
implementation libs.slf4jApi
implementation libs.jacksonDatabindYaml
Copy link
Member

Choose a reason for hiding this comment

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

this is required as test-common needs to parse yaml in production code.

build.gradle Outdated
@@ -1588,7 +1588,6 @@ project(':test-common') {
implementation project(':storage')
implementation project(':server-common')
implementation libs.slf4jApi
Copy link
Member

Choose a reason for hiding this comment

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

Could you please take a look at @ijuma's comment (#17373 (comment))

maybe we can create a variable to collect them and then add them to implementation for each module:

  log4jRuntimeLibs = [
    libs.log4j1Bridge2Api
    libs.jacksonDatabindYaml
  ]
  log4jLibs = [
    libs.slf4jApi
    libs.slf4jLog4j2
    libs.log4j2Api
    libs.log4j2Core
  ]
...
implementation log4jLibs
runtimeOnly log4jRuntimeLibs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried it on my local machine but it led to tons of failure of tests.
I would keep debugging on my local machine.

docs/upgrade.html
@@ -227,6 +227,11 @@ <h5><a id="upgrade_400_notable" href="#upgrade_400_notable">Notable changes in 4
Java 8 support has been removed in Apache Kafka 4.0
See <a href="https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=181308223">KIP-750</a> for more details
</li>
<li>
Logging framework has been migrated from Log4j to Log4j2.
Copy link
Member

Choose a reason for hiding this comment

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

Log4j provides a way to convert Log4j 1.x properties at runtime (Log4j Migration Guide).

To enable this feature, we can set both LOG4J_COMPATIBILITY=true and specify the configuration file using LOG4J_CONFIGURATION_FILE=file.

@frankvicky frankvicky force-pushed the log4j2-follow-up branch 2 times, most recently from c95cd3c to c69d421 Compare December 23, 2024 16:14
@github-actions github-actions bot removed the small Small PRs label Dec 23, 2024
ijuma
ijuma reviewed Dec 24, 2024
View reviewed changes
build.gradle Outdated
implementation libs.opentelemetryProto
implementation libs.protobuf
implementation log4jLibs
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, clients and streams should not have any log dependency besides slf4j. The idea is that client modules shouldn't include logging implementations while server modules should. Client modules run alongside user applications and the logging library should be aligned with the application logging library.

chia7712
chia7712 reviewed Dec 24, 2024
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@frankvicky could you please also remove log4j2CoreTest? it is unused.

config/tools-log4j2.yaml Outdated
@@ -12,10 +12,20 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
Configuration:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please open a jira for this change? tools-log4j.properties is still used in code base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
https://issues.apache.org/jira/browse/KAFKA-18347

build.gradle
@@ -1647,6 +1657,9 @@ project(':test-common:test-common-runtime') {
implementation libs.junitPlatformLanucher
implementation libs.junitJupiterApi
implementation libs.junitJupiter
implementation log4jLibs
Copy link
Member

Choose a reason for hiding this comment

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

line#1656 implementation libs.slf4jApi is unnecessary now.

@frankvicky frankvicky force-pushed the log4j2-follow-up branch 2 times, most recently from b4094cd to e6000ec Compare December 24, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chia7712 chia7712 chia7712 left review comments

@ijuma ijuma ijuma left review comments

Assignees
No one assigned
Labels
build Gradle build or GitHub Actions ci-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frankvicky @chia7712 @ijuma