You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "frankvicky (via GitHub)" <gi...@apache.org> on 2024/12/21 03:17:08 UTC
[PR] MINOR: log4j2 upgrade follow-up [kafka]
frankvicky opened a new pull request, #18290:
URL: https://github.com/apache/kafka/pull/18290
Please refer to https://github.com/apache/kafka/pull/17373#issuecomment-2557311234 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #18290:
URL: https://github.com/apache/kafka/pull/18290#issuecomment-2564633215
Hi @chia7712
I have tested log4j compatibility on my local machine.
Explicit set `KAFKA_LOG4J_OPTS`:
Leave `KAFKA_LOG4J_OPTS` as null:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1898913342
##########
bin/kafka-run-class.sh:
##########
@@ -225,6 +225,18 @@ if [ -z "$KAFKA_LOG4J_OPTS" ]; then
(( WINDOWS_OS_FORMAT )) && LOG4J_DIR=$(cygpath --path --mixed "${LOG4J_DIR}")
KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:${LOG4J_DIR}"
else
+ if echo "$KAFKA_LOG4J_OPTS" | grep -qE "log4j\.[^[:space:]]+$"; then
+ # Extract Log4j 1.x configuration file path from KAFKA_LOG4J_OPTS
+ LOG4J1_CONFIG=$(echo "$KAFKA_LOG4J_OPTS" | grep -o 'log4j\.configuration=\S*' | cut -d'=' -f2)
+
+ # Enable Log4j 1.x configuration compatibility mode for Log4j 2
+ export LOG4J_COMPATIBILITY=true
Review Comment:
From document, set one of confituration from `log4j1.compatibility` and `log4j.configuration` if needing to install log4j1 to log4j2 bridge. It looks like we can set `LOG4J_COMPATIBILITY=true` only if `LOG4J1_CONFIG` has properties file. Also, setting `LOG4J_CONFIGURATION_FILE="$LOG4J1_CONFIG"` only if `LOG4J1_CONFIG` doesn't have properties file.
https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#ConfigurationCompatibility
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #18290:
URL: https://github.com/apache/kafka/pull/18290
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1899151139
##########
build.gradle:
##########
@@ -182,6 +194,7 @@ allprojects {
// ZooKeeper (potentially older and containing CVEs)
libs.nettyHandler,
libs.nettyTransportNativeEpoll,
+ libs.slf4jApi,
Review Comment:
why we need this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1899153847
##########
build.gradle:
##########
@@ -182,6 +194,7 @@ allprojects {
// ZooKeeper (potentially older and containing CVEs)
libs.nettyHandler,
libs.nettyTransportNativeEpoll,
+ libs.slf4jApi,
Review Comment:
I previously encountered version inconsistency issues on my local machine. While those issues have been resolved, this change represents a modification that should have been removed but was accidentally missed during cleanup. 😬
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1894804402
##########
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.
Review Comment:
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+.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1899153025
##########
build.gradle:
##########
@@ -182,6 +194,7 @@ allprojects {
// ZooKeeper (potentially older and containing CVEs)
libs.nettyHandler,
libs.nettyTransportNativeEpoll,
+ libs.slf4jApi,
Review Comment:
```
spotbugsSlf4j - configuration for the SLF4J provider to run SpotBugs
\--- org.slf4j:slf4j-simple:2.0.0
\--- org.slf4j:slf4j-api:2.0.0
```
spotbugs requires the 2.x slf4j, so forcing the version can cause log issue in running spotbugs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1896536420
##########
config/tools-log4j2.yaml:
##########
@@ -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:
Review Comment:
Could you please open a jira for this change? `tools-log4j.properties` is still used in code base
##########
build.gradle:
##########
@@ -1647,6 +1657,9 @@ project(':test-common:test-common-runtime') {
implementation libs.junitPlatformLanucher
implementation libs.junitJupiterApi
implementation libs.junitJupiter
+ implementation log4jLibs
Review Comment:
line#1656 `implementation libs.slf4jApi` is unnecessary now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1894978437
##########
build.gradle:
##########
@@ -1588,7 +1588,6 @@ project(':test-common') {
implementation project(':storage')
implementation project(':server-common')
implementation libs.slf4jApi
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1896350581
##########
build.gradle:
##########
@@ -1881,28 +1901,26 @@ project(':clients') {
implementation libs.zstd
implementation libs.lz4
implementation libs.snappy
- implementation libs.slf4jApi
implementation libs.opentelemetryProto
implementation libs.protobuf
+ implementation log4jLibs
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1894880050
##########
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.
Review Comment:
Even better if it can be always enabled with no significant downside.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1896582502
##########
config/tools-log4j2.yaml:
##########
@@ -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:
Review Comment:
Sure.
https://issues.apache.org/jira/browse/KAFKA-18347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1894767983
##########
build.gradle:
##########
@@ -1588,7 +1588,6 @@ project(':test-common') {
implementation project(':storage')
implementation project(':server-common')
implementation libs.slf4jApi
Review Comment:
Could you please take a look at @ijuma's comment (https://github.com/apache/kafka/pull/17373#discussion_r1894553658)
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
```
##########
build.gradle:
##########
@@ -1588,7 +1588,6 @@ project(':test-common') {
implementation project(':storage')
implementation project(':server-common')
implementation libs.slf4jApi
- implementation libs.jacksonDatabindYaml
Review Comment:
this is required as `test-common` needs to parse yaml in production code.
##########
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.
Review Comment:
Log4j provides a way to convert Log4j 1.x properties at runtime ([Log4j Migration Guide](https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#ConfigurationCompatibility)).
To enable this feature, we can set both `LOG4J_COMPATIBILITY=true` and specify the configuration file using `LOG4J_CONFIGURATION_FILE=file`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1894831661
##########
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.
Review Comment:
We can always enable that compatibility feature as the bridge jar is already included now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18290:
URL: https://github.com/apache/kafka/pull/18290#issuecomment-2557975691
@frankvicky as @ijuma's comment in https://issues.apache.org/jira/browse/KAFKA-18247, please add the [`log4j2 transform ci`](https://logging.staged.apache.org/log4j/transform/cli.html#log4j-transform-cli) to the upgrade note.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1894554214
##########
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.
Review Comment:
Shall we mention the behavior if you do _not_ convert the configuration files? I believe there is a compatibility mode with some limitations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: log4j2 upgrade follow-up [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18290:
URL: https://github.com/apache/kafka/pull/18290#discussion_r1898896277
##########
bin/kafka-run-class.sh:
##########
@@ -225,6 +225,18 @@ if [ -z "$KAFKA_LOG4J_OPTS" ]; then
(( WINDOWS_OS_FORMAT )) && LOG4J_DIR=$(cygpath --path --mixed "${LOG4J_DIR}")
KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:${LOG4J_DIR}"
else
+ if echo "$KAFKA_LOG4J_OPTS" | grep -qE "log4j\.[^[:space:]]+$"; then
+ # Extract Log4j 1.x configuration file path from KAFKA_LOG4J_OPTS
+ LOG4J1_CONFIG=$(echo "$KAFKA_LOG4J_OPTS" | grep -o 'log4j\.configuration=\S*' | cut -d'=' -f2)
+
+ # Enable Log4j 1.x configuration compatibility mode for Log4j 2
+ export LOG4J_COMPATIBILITY=true
Review Comment:
@frankvicky could you please test the compatibility on your local?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org