chia7712 commented on code in PR #17373:
URL: https://github.com/apache/kafka/pull/17373#discussion_r1864435870
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -805,7 +805,7 @@ def start_cmd(self, node):
kafka_mode = self.context.globals.get("kafka_mode", "")
cmd = f"export KAFKA_MODE={kafka_mode}; "
cmd += "export JMX_PORT=%d; " % self.jmx_port
- cmd += "export KAFKA_LOG4J_OPTS=\"-Dlog4j.configuration=file:%s\"; " %
self.LOG4J_CONFIG
+ cmd += "export KAFKA_LOG4J_OPTS=\"%s%s\"; " %
(get_log4j_config_param(node), get_log4j_config_for_kafka(node))
Review Comment:
ditto. use `os.path.join(self.PERSISTENT_ROOT, get_log4j_config(node))`
instead
##########
tests/kafkatest/services/connect.py:
##########
@@ -400,7 +399,7 @@ def __init__(self, context, num_nodes, kafka, files,
offsets_topic="connect-offs
# connector_configs argument is intentionally ignored in distributed
service.
def start_cmd(self, node, connector_configs):
- cmd = "( export KAFKA_LOG4J_OPTS=\"-Dlog4j.configuration=file:%s\"; "
% self.LOG4J_CONFIG_FILE
+ cmd = "( export KAFKA_LOG4J_OPTS=\"%s%s\"; " %
(get_log4j_config_param(node), get_log4j_config_for_connect(node))
Review Comment:
ditto
##########
tests/kafkatest/services/kafka/util.py:
##########
@@ -30,4 +31,23 @@ def fix_opts_for_new_jvm(node):
return ""
+def get_log4j_config_param(node):
+ return '-Dlog4j2.configurationFile=file:' if get_version(node) >=
LATEST_4_0 else '-Dlog4j.configuration=file:'
+def get_log4j_config(node):
+ return 'log4j2.yaml' if get_version(node) >= LATEST_4_0 else
'log4j.properties'
+
+def get_log4j_config_for_kafka(node):
Review Comment:
I don't think those configs file are existent. please remove it
##########
bin/connect-mirror-maker.sh:
##########
@@ -22,8 +22,15 @@ fi
base_dir=$(dirname $0)
-if [ "x$KAFKA_LOG4J_OPTS" = "x" ]; then
+if [ -f "$base_dir/../config/connect-log4j.properties" ]; then
+ echo DEPRECATED: Using Log4j 1.x configuration file
\$KAFKA_HOME/config/connect-log4j.properties >&2
+ echo To use a Log4j 2.x configuration, please see
https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#Log4j2ConfigurationFormat
for details about Log4j configuration file migration. >&2
+ echo You can also use the \$KAFKA_HOME/config/connect-log4j2.yaml file as
a starting point. Make sure to remove the Log4j 1.x configuration after
completing the migration. >&2
export
KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:$base_dir/../config/connect-log4j.properties"
+elif [ -f "$base_dir/../config/connect-log4j2.yaml" ]; then
Review Comment:
ditto
##########
tests/kafkatest/services/connect.py:
##########
@@ -340,7 +339,7 @@ def node(self):
return self.nodes[0]
def start_cmd(self, node, connector_configs):
- cmd = "( export KAFKA_LOG4J_OPTS=\"-Dlog4j.configuration=file:%s\"; "
% self.LOG4J_CONFIG_FILE
+ cmd = "( export KAFKA_LOG4J_OPTS=\"%s%s\"; " %
(get_log4j_config_param(node), get_log4j_config_for_connect(node))
Review Comment:
`get_log4j_config_for_connect(node)` does not point to the "full" path, so
all related services can't start up as it fails to find the log4j2 config.
Please use `os.path.join(self.PERSISTENT_ROOT,
get_log4j_config_for_connect(node))` instead
##########
tests/kafkatest/services/connect.py:
##########
@@ -364,7 +363,7 @@ def start_node(self, node, **kwargs):
if self.external_config_template_func:
node.account.create_file(self.EXTERNAL_CONFIGS_FILE,
self.external_config_template_func(node))
node.account.create_file(self.CONFIG_FILE,
self.config_template_func(node))
- node.account.create_file(self.LOG4J_CONFIG_FILE,
self.render('connect_log4j.properties', log_file=self.LOG_FILE))
+ node.account.create_file(get_log4j_config_for_connect(node),
self.render(get_log4j_config_for_connect(node), log_file=self.LOG_FILE))
Review Comment:
ditto. `get_log4j_config_for_connect(node)` is a file name rather than full
path. please use `os.path.join(self.PERSISTENT_ROOT,
get_log4j_config_for_connect(node))` instead
##########
tests/kafkatest/services/kafka/kafka.py:
##########
@@ -874,7 +874,7 @@ def start_node(self, node, timeout_sec=60, **kwargs):
self.logger.info("kafka.properties:")
self.logger.info(prop_file)
node.account.create_file(KafkaService.CONFIG_FILE, prop_file)
- node.account.create_file(self.LOG4J_CONFIG,
self.render('log4j.properties', log_dir=KafkaService.OPERATIONAL_LOG_DIR))
+ node.account.create_file(get_log4j_config_for_kafka(node),
self.render(get_log4j_config(node), log_dir=KafkaService.OPERATIONAL_LOG_DIR))
Review Comment:
ditto. use `os.path.join(self.PERSISTENT_ROOT, get_log4j_config(node))`
instead
##########
bin/kafka-server-start.sh:
##########
@@ -22,7 +22,7 @@ fi
base_dir=$(dirname $0)
if [ "x$KAFKA_LOG4J_OPTS" = "x" ]; then
- export
KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:$base_dir/../config/log4j.properties"
Review Comment:
I’m not sure why we override the `KAFKA_LOG4J_OPTS` here. We typically allow
users to define custom `KAFKA_LOG4J_OPTS`. Moreover, overriding
`KAFKA_LOG4J_OPTS` can break many end-to-end tests, as they often create log4j
configurations dynamically and pass them through `KAFKA_LOG4J_OPTS`
Noted that we do not require users to strictly use the path
`$base_dir/../config/log4j2.xml`.
##########
bin/connect-distributed.sh:
##########
@@ -22,8 +22,15 @@ fi
base_dir=$(dirname $0)
-if [ "x$KAFKA_LOG4J_OPTS" = "x" ]; then
+if [ -f "$base_dir/../config/connect-log4j.properties" ]; then
+ echo DEPRECATED: Using Log4j 1.x configuration file
\$KAFKA_HOME/config/connect-log4j.properties >&2
+ echo To use a Log4j 2.x configuration, please see
https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#Log4j2ConfigurationFormat
for details about Log4j configuration file migration. >&2
+ echo You can also use the \$KAFKA_HOME/config/connect-log4j2.yaml file as
a starting point. Make sure to remove the Log4j 1.x configuration after
completing the migration. >&2
export
KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:$base_dir/../config/connect-log4j.properties"
+elif [ -f "$base_dir/../config/connect-log4j2.yaml" ]; then
Review Comment:
why do we override the config path? it breaks all e2e since the custom log4j
config can't be used.
##########
bin/connect-standalone.sh:
##########
@@ -22,8 +22,15 @@ fi
base_dir=$(dirname $0)
-if [ "x$KAFKA_LOG4J_OPTS" = "x" ]; then
+if [ -f "$base_dir/../config/connect-log4j.properties" ]; then
+ echo DEPRECATED: Using Log4j 1.x configuration file
\$KAFKA_HOME/config/connect-log4j.properties >&2
+ echo To use a Log4j 2.x configuration, please see
https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#Log4j2ConfigurationFormat
for details about Log4j configuration file migration. >&2
+ echo You can also use the \$KAFKA_HOME/config/connect-log4j2.yaml file as
a starting point. Make sure to remove the Log4j 1.x configuration after
completing the migration. >&2
export
KAFKA_LOG4J_OPTS="-Dlog4j.configuration=file:$base_dir/../config/connect-log4j.properties"
+elif [ -f "$base_dir/../config/connect-log4j2.yaml" ]; then
Review Comment:
ditto
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
