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 2025/01/06 12:21:51 UTC
[PR] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
frankvicky opened a new pull request, #18394:
URL: https://github.com/apache/kafka/pull/18394
JIRA: KAFKA-18396
After log4j migration, we need to update the logging configuration in KafkaDockerWrapper from log4j1 to 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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "omkreddy (via GitHub)" <gi...@apache.org>.
omkreddy commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2647695006
@frankvicky Sorry, my bad. I might have overwritten your changes. Yes, the changes look good to me. Please reapply these changes to this patch.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "omkreddy (via GitHub)" <gi...@apache.org>.
omkreddy merged PR #18394:
URL: https://github.com/apache/kafka/pull/18394
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2588820131
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "omkreddy (via GitHub)" <gi...@apache.org>.
omkreddy commented on code in PR #18394:
URL: https://github.com/apache/kafka/pull/18394#discussion_r1946442355
##########
core/src/main/java/kafka/docker/Log4jConfiguration.java:
##########
@@ -0,0 +1,353 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.
+ */
+package kafka.docker;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+public class Log4jConfiguration {
Review Comment:
@frankvicky Thanks for the PR. Is it possible to use Log4j2 own classes for this
https://github.com/apache/logging-log4j2/tree/2.x/log4j-core/src/main/java/org/apache/logging/log4j/core/config/yaml
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2638723667
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18394:
URL: https://github.com/apache/kafka/pull/18394#discussion_r1959193091
##########
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##########
@@ -219,6 +231,68 @@ object KafkaDockerWrapper extends Logging {
Files.write(filepath, properties.getBytes(StandardCharsets.UTF_8), mode)
}
+ private def addToYaml(loggerFromEnv: Array[Logger], rootOption: Option[Root], filepath: Path): Unit = {
+ val path = filepath
+ if (!Files.exists(path)) {
+ Files.createFile(path)
+ }
+
+ val mapper = new ObjectMapper(new YAMLFactory().disable(Feature.WRITE_DOC_START_MARKER))
+ .configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true)
+ .setSerializationInclusion(JsonInclude.Include.NON_EMPTY)
+ .findAndRegisterModules();
+
+ val yaml = try {
+ mapper.readValue(filepath.toFile, classOf[Log4jConfiguration])
+ } catch {
+ case _: MismatchedInputException => new Log4jConfiguration
+ case e: RuntimeException => throw e
+ }
+ val config = yaml.getConfiguration
+
+ if (config == null && loggerFromEnv.isEmpty && rootOption.isEmpty) {
Review Comment:
it can be fixed by #18872
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18394:
URL: https://github.com/apache/kafka/pull/18394#discussion_r1947539654
##########
core/src/main/java/kafka/docker/Log4jConfiguration.java:
##########
@@ -0,0 +1,353 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.
+ */
+package kafka.docker;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+public class Log4jConfiguration {
Review Comment:
Hi @omkreddy
Thanks for the information. 🙇🏼
I have looked into these files, but they don't seem to provide a way to write the logger configuration as a config 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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2647732888
No worries. I very much appreciate your review. 🙇🏼
Just reapply the changes as mentioned.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18394:
URL: https://github.com/apache/kafka/pull/18394#discussion_r1959192770
##########
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##########
@@ -219,6 +231,68 @@ object KafkaDockerWrapper extends Logging {
Files.write(filepath, properties.getBytes(StandardCharsets.UTF_8), mode)
}
+ private def addToYaml(loggerFromEnv: Array[Logger], rootOption: Option[Root], filepath: Path): Unit = {
+ val path = filepath
+ if (!Files.exists(path)) {
+ Files.createFile(path)
+ }
+
+ val mapper = new ObjectMapper(new YAMLFactory().disable(Feature.WRITE_DOC_START_MARKER))
+ .configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true)
+ .setSerializationInclusion(JsonInclude.Include.NON_EMPTY)
+ .findAndRegisterModules();
+
+ val yaml = try {
+ mapper.readValue(filepath.toFile, classOf[Log4jConfiguration])
+ } catch {
+ case _: MismatchedInputException => new Log4jConfiguration
+ case e: RuntimeException => throw e
+ }
+ val config = yaml.getConfiguration
+
+ if (config == null && loggerFromEnv.isEmpty && rootOption.isEmpty) {
Review Comment:
we should remove this condition. otherwise, the script used by docker can't see the output log4j2 config ...
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #18394:
URL: https://github.com/apache/kafka/pull/18394#discussion_r1948749017
##########
core/src/main/java/kafka/docker/Log4jConfiguration.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.
+ */
+package kafka.docker;
+
+import com.fasterxml.jackson.annotation.JsonAnyGetter;
+import com.fasterxml.jackson.annotation.JsonAnySetter;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonPropertyOrder;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class Log4jConfiguration {
+ private Configuration configuration;
+
+ @JsonProperty("Configuration")
+ public Configuration getConfiguration() {
+ return configuration;
+ }
+
+ public void setConfiguration(Configuration configuration) {
+ this.configuration = configuration;
+ }
+}
+
+@JsonPropertyOrder({ "Properties", "Appenders", "Loggers" })
+@JsonIgnoreProperties(ignoreUnknown = true)
+class Configuration {
+ private Properties properties;
+ private Appenders appenders;
+ private Loggers loggers;
+ private final Map<String, Object> additionalProperties = new LinkedHashMap<>();
+
+ @JsonProperty("Properties")
+ public Properties getProperties() {
+ return properties;
+ }
+
+ public void setProperties(Properties properties) {
+ this.properties = properties;
+ }
+
+ @JsonProperty("Appenders")
+ public Appenders getAppenders() {
+ return appenders;
+ }
+
+ public void setAppenders(Appenders appenders) {
+ this.appenders = appenders;
+ }
+
+ @JsonProperty("Loggers")
+ public Loggers getLoggers() {
+ return loggers;
+ }
+
+ public void setLoggers(Loggers loggers) {
+ this.loggers = loggers;
+ }
+
+ @JsonAnyGetter
+ public Map<String, Object> getAdditionalProperties() {
+ return additionalProperties;
+ }
+
+ @JsonAnySetter
+ public void setAdditionalProperties(String key, Object value) {
+ additionalProperties.put(key, value);
+ }
+}
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+class Properties {
+ private final Map<String, Object> properties = new LinkedHashMap<>();
+
+ @JsonAnyGetter
+ public Map<String, Object> getProperties() {
+ return properties;
+ }
+
+ @JsonAnySetter
+ public void setProperties(String key, Object value) {
+ properties.put(key, value);
+ }
+}
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+class Appenders {
+ private final Map<String, Object> properties = new LinkedHashMap<>();
+
+ @JsonAnyGetter
+ public Map<String, Object> getProperties() {
+ return properties;
+ }
+
+ @JsonAnySetter
+ public void setProperties(String key, Object value) {
+ properties.put(key, value);
+ }
+}
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+class Loggers {
+ private Root root;
+ private List<Logger> logger = Collections.emptyList();
+
+ @JsonProperty("Root")
+ public Root getRoot() {
+ return root;
+ }
+
+ public void setRoot(Root root) {
+ this.root = root;
+ }
+
+ @JsonProperty("Logger")
+ public List<Logger> getLogger() {
+ return logger;
+ }
+
+ public void setLogger(List<Logger> logger) {
+ this.logger = logger;
+ }
+}
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+class Root {
+ private String level;
+ private final Map<String, Object> otherProperties = new LinkedHashMap<>();
+
+ public String getLevel() {
+ return level;
+ }
+
+ public void setLevel(String level) {
+ this.level = level;
+ }
+
+ @JsonAnyGetter
+ public Map<String, Object> getOtherProperties() {
+ return otherProperties;
+ }
+
+ @JsonAnySetter
+ public void setOtherProperties(String key, Object value) {
+ otherProperties.put(key, value);
+ }
+}
+
+@JsonPropertyOrder({ "name", "level" })
+@JsonIgnoreProperties(ignoreUnknown = true)
+class Logger {
+ private String name;
+ private String level;
+ private final Map<String, Object> otherProperties = new LinkedHashMap<>();
Review Comment:
`KAFKA_LOG4J_LOGGERS` is only used for setting logger names and levels, so we don't need to consider other properties here.
This property is for serialization purposes.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2594379624
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "ukpagrace (via GitHub)" <gi...@apache.org>.
ukpagrace commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2590150195
Hi, i would like to work on this.
--
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] KAFKA-18396: Migrate log4j1 configuration to log4j2 in KafkaDockerWrapper [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #18394:
URL: https://github.com/apache/kafka/pull/18394#issuecomment-2647646366
Hi @omkreddy,
Thanks for your help.
I noticed you've made some changes based on my previous version. In the earlier versions, we serialized several properties that we didn't actually manipulate in Kafka. Could you please review the recent changes to `Log4jConfiguration`? It minimizes the serialized properties. If you agree, I'll apply these changes to this patch soon.
https://github.com/apache/kafka/commit/4e56fb18074a38bae3a0e829efe471c252658bdd#diff-01ac885071d3bbefe2f9c96ff1d0d6597f4a6af1b2a9d2047deba341d5aa0e69
--
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