AndrewJSchofield commented on code in PR #20390:
URL: https://github.com/apache/kafka/pull/20390#discussion_r2297770384
##########
tests/kafkatest/services/verifiable_consumer.py:
##########
@@ -424,7 +424,7 @@ def start_cmd(self, node):
if self.max_messages > 0:
cmd += " --max-messages %s" % str(self.max_messages)
- cmd += " --consumer.config %s" % VerifiableConsumer.CONFIG_FILE
+ cmd += " --command.config %s" % VerifiableConsumer.CONFIG_FILE
Review Comment:
`--command-config`
##########
tests/kafkatest/services/verifiable_client.py:
##########
@@ -74,7 +74,8 @@
* `--enable-autocommit`
* `--max-messages <n>`
* `--assignment-strategy <s>`
- * `--consumer.config <config-file>` - consumer config properties (typically
empty)
+ * `--consumer.config <config-file>` - (DEPRECATED) consumer config properties
(typically empty). This option will be removed in a future version. Use
--command-config instead.
+ * `--command.config <config-file>` - consumer config properties
Review Comment:
Shouldn't this be `--command-config`?
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableConsumer.java:
##########
@@ -622,16 +631,27 @@ public static VerifiableConsumer
createFromArgs(ArgumentParser parser, String[]
boolean useAutoCommit = res.getBoolean("useAutoCommit");
String configFile = res.getString("consumer.config");
+ String commandConfigFile = res.getString("command-config");
String brokerHostAndPort = res.getString("bootstrapServer");
Properties consumerProps = new Properties();
+ if (configFile != null && commandConfigFile != null) {
+ throw new ArgumentParserException("Options --consumer.config and
--command-config are mutually exclusive.", parser);
+ }
if (configFile != null) {
try {
consumerProps.putAll(Utils.loadProps(configFile));
} catch (IOException e) {
throw new ArgumentParserException(e.getMessage(), parser);
}
}
+ if (res.getString(commandConfigFile) != null) {
Review Comment:
Can't you just use `if (commandConfigFile != null)` here? And also simplify
the `loadProps` line accordingly?
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableConsumer.java:
##########
@@ -612,7 +612,16 @@ private static ArgumentParser argParser() {
.required(false)
.type(String.class)
.metavar("CONFIG_FILE")
- .help("Consumer config properties file (config options shared
with command line parameters will be overridden).");
+ .help("(DEPRECATED) Consumer config properties file (config
options shared with command line parameters will be overridden)." +
+ "This option will be removed in a future version. Use
--command-config instead.");
+
+ parser.addArgument("--command-config")
+ .action(store())
+ .required(false)
+ .type(String.class)
+ .metavar("CONFIG-FILE")
+ .dest("commandConfigFile")
+ .help("Consumer config properties file.");
Review Comment:
Given that we are trying in this KIP to get away from specific names for
producer/config config, maybe this could be `"Config properties file.`.
##########
tests/kafkatest/services/verifiable_client.py:
##########
@@ -97,7 +98,8 @@
* `--broker-list <brokers>`
* `--max-messages <n>`
* `--throughput <msgs/s>`
- * `--producer.config <config-file>` - producer config properties (typically
empty)
+ * `--producer.config <config-file>` - producer config properties (typically
empty). This option will be removed in a future version. Use --command-config
instead.
+ * `--command.config <config-file>` - producer config properties
Review Comment:
Shouldn't this be `--command-config` also?
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableConsumer.java:
##########
@@ -622,16 +631,27 @@ public static VerifiableConsumer
createFromArgs(ArgumentParser parser, String[]
boolean useAutoCommit = res.getBoolean("useAutoCommit");
String configFile = res.getString("consumer.config");
+ String commandConfigFile = res.getString("command-config");
String brokerHostAndPort = res.getString("bootstrapServer");
Properties consumerProps = new Properties();
+ if (configFile != null && commandConfigFile != null) {
+ throw new ArgumentParserException("Options --consumer.config and
--command-config are mutually exclusive.", parser);
+ }
if (configFile != null) {
try {
Review Comment:
Please add a deprecation message here like `System.out.println("Option
--consumer.config has been deprecated and will be removed in a future version.
Use --command-config instead.");`.
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableConsumer.java:
##########
@@ -622,16 +631,27 @@ public static VerifiableConsumer
createFromArgs(ArgumentParser parser, String[]
boolean useAutoCommit = res.getBoolean("useAutoCommit");
String configFile = res.getString("consumer.config");
+ String commandConfigFile = res.getString("command-config");
Review Comment:
Since you used the destination of `commandConfigFile` on line 623, I would
expect you to refer to the string by that name here, unless I'm missing
something.
##########
tests/kafkatest/services/verifiable_producer.py:
##########
@@ -249,7 +249,7 @@ def start_cmd(self, node, idx):
if self.repeating_keys is not None:
cmd += " --repeating-keys %s " % str(self.repeating_keys)
- cmd += " --producer.config %s" % VerifiableProducer.CONFIG_FILE
+ cmd += " --command.config %s" % VerifiableProducer.CONFIG_FILE
Review Comment:
`--command-config`
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableProducer.java:
##########
@@ -189,6 +190,14 @@ private static ArgumentParser argParser() {
.dest("repeatingKeys")
.help("If specified, each produced record will have a key starting
at 0 increment by 1 up to the number specified (exclusive), then the key is set
to 0 again");
+ parser.addArgument("--command-config")
+ .action(store())
+ .required(false)
+ .type(String.class)
+ .metavar("CONFIG-FILE")
+ .dest("commandConfigFile")
+ .help("Producer config properties file.");
Review Comment:
Given that we are trying in this KIP to get away from specific names for
producer/config config, maybe this could be `"Config properties file.`.
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableProducer.java:
##########
@@ -189,6 +190,14 @@ private static ArgumentParser argParser() {
.dest("repeatingKeys")
.help("If specified, each produced record will have a key starting
at 0 increment by 1 up to the number specified (exclusive), then the key is set
to 0 again");
+ parser.addArgument("--command-config")
+ .action(store())
+ .required(false)
+ .type(String.class)
+ .metavar("CONFIG-FILE")
Review Comment:
The names of the metavars use a mixture of `-` and `_` as separators. Please
could you choose one or the other (I have a slight preference for `-`), and
then apply to the delinquent arguments.
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableConsumer.java:
##########
@@ -612,7 +612,16 @@ private static ArgumentParser argParser() {
.required(false)
.type(String.class)
.metavar("CONFIG_FILE")
- .help("Consumer config properties file (config options shared
with command line parameters will be overridden).");
+ .help("(DEPRECATED) Consumer config properties file (config
options shared with command line parameters will be overridden)." +
+ "This option will be removed in a future version. Use
--command-config instead.");
+
+ parser.addArgument("--command-config")
+ .action(store())
+ .required(false)
+ .type(String.class)
+ .metavar("CONFIG-FILE")
Review Comment:
The names of the metavars use a mixture of `-` and `_` as separators. Please
could you choose one or the other (I have a slight preference for `-`), and
then apply to the delinquent arguments.
##########
tools/src/main/java/org/apache/kafka/tools/VerifiableProducer.java:
##########
@@ -240,14 +250,25 @@ public static VerifiableProducer
createFromArgs(ArgumentParser parser, String[]
producerProps.put(ProducerConfig.ACKS_CONFIG,
Integer.toString(res.getInt("acks")));
// No producer retries
producerProps.put(ProducerConfig.RETRIES_CONFIG, "0");
+ if (configFile != null && commandConfigFile != null) {
+ throw new ArgumentParserException("Options --producer.config and
--command-config are mutually exclusive.", parser);
+ }
+
if (configFile != null) {
+ System.out.println("Option --producer-config has been deprecated
and will be removed in a future version. Use --command-config instead.");
Review Comment:
`--producer.config has been deprecated`....
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
