KAFKA-19625: Consistency of command-line arguments for verifiable producer/consumer by JimmyWang6 · Pull Request #20390 · apache/kafka

JimmyWang6 · 2025-08-21T09:13:52Z

As described in
jira, this PR
implements replace consumer.config and producer.config with
command-config for kafka-verifiable-producer.sh and
kafka-verifiable-consumer.sh.

Reviewers: Andrew Schofield aschofield@confluent.io

AndrewJSchofield

Thanks for the PR.

AndrewJSchofield · 2025-08-25T10:47:06Z

            cmd += " --repeating-keys %s " % str(self.repeating_keys)

-        cmd += " --producer.config %s" % VerifiableProducer.CONFIG_FILE
+        cmd += " --command.config %s" % VerifiableProducer.CONFIG_FILE


--command-config

AndrewJSchofield · 2025-08-25T10:48:31Z


        boolean useAutoCommit = res.getBoolean("useAutoCommit");
        String configFile = res.getString("consumer.config");
+        String commandConfigFile = res.getString("command-config");


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.

AndrewJSchofield · 2025-08-25T10:54:34Z

+                .action(store())
+                .required(false)
+                .type(String.class)
+                .metavar("CONFIG-FILE")


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.

AndrewJSchofield · 2025-08-25T10:56:52Z

+            .type(String.class)
+            .metavar("CONFIG-FILE")
+            .dest("commandConfigFile")
+            .help("Producer config properties file.");


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..

AndrewJSchofield · 2025-08-25T10:57:02Z

+                .type(String.class)
+                .metavar("CONFIG-FILE")
+                .dest("commandConfigFile")
+                .help("Consumer config properties file.");


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..

AndrewJSchofield · 2025-08-25T10:58:41Z

+            throw new ArgumentParserException("Options --consumer.config and --command-config are mutually exclusive.", parser);
+        }
        if (configFile != null) {
            try {


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.");.

I'm really sorry—I just woke up, so I made a lot of silly typo errors. :( I’ve already addressed all the comments. Thanks for your review.

Ah, not quite. The deprecation message should be printed if the user specified the old --consumer.config and we'd like to encourage them to use --command-config. So, I think the System.out.println you added ought to be added just after if (configFile != null) on line 642.

AndrewJSchofield

Thanks for the changes.

AndrewJSchofield · 2025-08-25T14:51:50Z

+            throw new ArgumentParserException("Options --consumer.config and --command-config are mutually exclusive.", parser);
+        }
        if (configFile != null) {
            try {


Ah, not quite. The deprecation message should be printed if the user specified the old --consumer.config and we'd like to encourage them to use --command-config. So, I think the System.out.println you added ought to be added just after if (configFile != null) on line 642.

JimmyWang6 · 2025-08-26T09:23:18Z

@AndrewJSchofield Thanks for your comments, I have already fixed them, please take a look.

AndrewJSchofield

Thanks for the updates. Just a couple of remaining comments.

AndrewJSchofield · 2025-08-26T13:34:05Z

+                .type(String.class)
+                .metavar("CONFIG-FILE")
+                .dest("commandConfigFile")
+                .help("Config properties file.");


I suggest adding (config options shared with command line parameters will be overridden) to the help string. That's still true.

AndrewJSchofield · 2025-08-26T13:34:24Z

+            .type(String.class)
+            .metavar("CONFIG-FILE")
+            .dest("commandConfigFile")
+            .help("Config properties file.");


And here (config options shared with command line parameters will be overridden).

chia7712

@JimmyWang6 Could you please file minor patch to fix the broken e2e? we could generate the command line according to the kafka version.

chia7712 · 2025-09-03T08:41:11Z

            cmd += " --repeating-keys %s " % str(self.repeating_keys)

-        cmd += " --producer.config %s" % VerifiableProducer.CONFIG_FILE
+        cmd += " --command-config %s" % VerifiableProducer.CONFIG_FILE


this change could break the compatibility_test_new_broker_test.py, since the VerifiableProducer is running under old kafka.

upgrade_test.py will also fail with these changes.

cc @JimmyWang6

@chia7712 @omkreddy Thanks for this. These tests are an area of the code which I am unfamiliar with. Next time I will be aware.

chia7712 · 2025-09-03T08:41:24Z

            cmd += " --max-messages %s" % str(self.max_messages)

-        cmd += " --consumer.config %s" % VerifiableConsumer.CONFIG_FILE
+        cmd += " --command-config %s" % VerifiableConsumer.CONFIG_FILE


this breaks consumer_protocol_migration_test.py

omkreddy · 2025-09-04T10:54:49Z

        }
+        if (commandConfigFile != null) {
+            try {
+                consumerProps.putAll(Utils.loadProps(res.getString(commandConfigFile)));


this should be
consumerProps.putAll(Utils.loadProps(commandConfigFile)); ?

…test.py (#20471) #20390 Replace the -`-producer.config` for the verifiable producer and `--consumer.config` option by `--command-config` for the verifiable consumer. However, for e2e tests targeting older broker versions, the original configuration should still be used. Fix the following tests: `consumer_protocol_migration_test.py`、`compatibility_test_new_broker_test.py` and `upgrade_test.py`. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Manikumar Reddy <manikumar.reddy@gmail.com>

…ducer/consumer (apache#20390) As described in [jira](https://issues.apache.org/jira/browse/KAFKA-19625), this PR implements replace `consumer.config` and `producer.config` with `command-config` for kafka-verifiable-producer.sh and kafka-verifiable-consumer.sh. Reviewers: Andrew Schofield <aschofield@confluent.io>

…test.py (apache#20471) apache#20390 Replace the -`-producer.config` for the verifiable producer and `--consumer.config` option by `--command-config` for the verifiable consumer. However, for e2e tests targeting older broker versions, the original configuration should still be used. Fix the following tests: `consumer_protocol_migration_test.py`、`compatibility_test_new_broker_test.py` and `upgrade_test.py`. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Manikumar Reddy <manikumar.reddy@gmail.com>

github-actions Bot added triage PRs from the community tools small Small PRs labels Aug 21, 2025

JimmyWang6 force-pushed the KAFKA-19625 branch from 671c683 to 3ed893b Compare August 21, 2025 09:36

JimmyWang6 added 2 commits August 25, 2025 10:03

Consistency of command-line arguments for verifiable producer/consumer

d2d51fe

Consistency of command-line arguments for verifiable producer/consumer

211e9c5

JimmyWang6 force-pushed the KAFKA-19625 branch from 3ed893b to 211e9c5 Compare August 25, 2025 03:16

Consistency of command-line arguments for verifiable producer/consumer

3863c85

AndrewJSchofield self-requested a review August 25, 2025 10:42

AndrewJSchofield removed the triage PRs from the community label Aug 25, 2025

AndrewJSchofield requested changes Aug 25, 2025

View reviewed changes

JimmyWang6 added 2 commits August 25, 2025 21:00

address comments

dc5135e

address comments

2aeb62a

AndrewJSchofield requested changes Aug 25, 2025

View reviewed changes

AndrewJSchofield added the ci-approved label Aug 25, 2025

address comments

f1096ef

JimmyWang6 requested a review from AndrewJSchofield August 26, 2025 09:23

AndrewJSchofield requested changes Aug 26, 2025

View reviewed changes

JimmyWang6 added 2 commits August 27, 2025 14:27

address comment

17f0878

fix typo

ad7a696

JimmyWang6 requested a review from AndrewJSchofield August 27, 2025 08:25

AndrewJSchofield approved these changes Aug 27, 2025

View reviewed changes

AndrewJSchofield merged commit a931f85 into apache:trunk Aug 27, 2025
21 checks passed

chia7712 reviewed Sep 3, 2025

View reviewed changes

JimmyWang6 mentioned this pull request Sep 3, 2025

MINOR: Fix failed e2e compatibility_test_new_broker_test and upgrade_test.py #20471

Merged

omkreddy reviewed Sep 4, 2025

View reviewed changes

Uh oh!

Conversation

JimmyWang6 commented Aug 21, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimmyWang6 Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimmyWang6 commented Aug 26, 2025

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JimmyWang6 commented Aug 21, 2025 •

edited by github-actions Bot

Loading

JimmyWang6 Aug 25, 2025 •

edited

Loading