Skip to content

KAFKA-19653: Improve metavariable names in usage messages #20438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2025
Merged

KAFKA-19653: Improve metavariable names in usage messages #20438

merged 1 commit into from
Sep 3, 2025

Conversation

AndrewJSchofield
Copy link
Member

@AndrewJSchofield AndrewJSchofield commented Aug 29, 2025 •
edited by github-actions bot
Loading

This trivial PR improves the so-called metavariable names in the usage
messages of the verifiable producer/consumer command-line tools. These
are the names of the replacement variables that appear solely in the
usage messages.

Verifiable producer (before):

usage: verifiable-producer [-h] --topic TOPIC
    [--max-messages MAX-MESSAGES] [--throughput THROUGHPUT]
    [--acks ACKS] [--producer.config CONFIG_FILE]
    [--message-create-time CREATETIME] [--value-prefix VALUE-PREFIX]
    [--repeating-keys REPEATING-KEYS] [--command-config CONFIG_FILE]
    --bootstrap-server HOST1:PORT1[,HOST2:PORT2[...]]

(after)

usage: verifiable-producer [-h] --topic TOPIC
    [--max-messages MAX-MESSAGES] [--throughput THROUGHPUT]
    [--acks ACKS] [--producer.config CONFIG-FILE]
    [--message-create-time CREATE-TIME] [--value-prefix VALUE-PREFIX]
    [--repeating-keys REPEATING-KEYS] [--command-config CONFIG-FILE]
    --bootstrap-server HOST1:PORT1[,HOST2:PORT2[...]]

Verifiable consumer (before):

usage: verifiable-consumer [-h] --topic TOPIC
    [--group-protocol GROUP_PROTOCOL]
    [--group-remote-assignor GROUP_REMOTE_ASSIGNOR]
    --group-id GROUP_ID
    [--group-instance-id GROUP_INSTANCE_ID]
    [--max-messages MAX-MESSAGES]
    [--session-timeout TIMEOUT_MS] [--verbose]
    [--enable-autocommit] [--reset-policy RESETPOLICY]
    [--assignment-strategy ASSIGNMENTSTRATEGY]
    [--consumer.config CONFIG_FILE] [--command-config CONFIG_FILE]
    --bootstrap-server HOST1:PORT1[,HOST2:PORT2[...]]

(after)

usage: verifiable-consumer [-h] --topic TOPIC
    [--group-protocol GROUP-PROTOCOL]
    [--group-remote-assignor GROUP-REMOTE-ASSIGNOR]
    --group-id GROUP-ID
    [--group-instance-id GROUP-INSTANCE-ID]
    [--max-messages MAX-MESSAGES]
    [--session-timeout TIMEOUT-MS] [--verbose]
    [--enable-autocommit] [--reset-policy RESET-POLICY]
    [--assignment-strategy ASSIGNMENT-STRATEGY]
    [--consumer.config CONFIG-FILE] [--command-config CONFIG-FILE]
    --bootstrap-server HOST1:PORT1[,HOST2:PORT2[...]]

Verifiable share consumer (before):

usage: verifiable-share-consumer
       [-h] --topic TOPIC --group-id GROUP_ID
       [--max-messages MAX-MESSAGES] [--verbose]
       [--acknowledgement-mode ACKNOWLEDGEMENTMODE]
       [--offset-reset-strategy OFFSETRESETSTRATEGY]
       [--command-config CONFIG_FILE]
       --bootstrap-server HOST1:PORT1[,HOST2:PORT2[...]]

(after):

usage: verifiable-share-consumer
       [-h] --topic TOPIC --group-id GROUP-ID
       [--max-messages MAX-MESSAGES] [--verbose]
       [--acknowledgement-mode ACKNOWLEDGEMENT-MODE]
       [--offset-reset-strategy OFFSET-RESET-STRATEGY]
       [--command-config CONFIG-FILE]
       --bootstrap-server HOST1:PORT1[,HOST2:PORT2[...]]

Reviewers: Kirk True [email protected], Ken Huang
[email protected], Lianet Magrans [email protected]

@github-actions github-actions bot added the tools label Aug 29, 2025
@Yunyung
Copy link
Collaborator

Yunyung commented Aug 29, 2025

LGTM. I see the same situation in Agent.java, Coordinator.java, MessageGenerator.java, etc., but it's outside the scope of this KIP. We can improve it in another PR.

m1a2st
m1a2st approved these changes Aug 29, 2025
View reviewed changes
Copy link
Collaborator

@m1a2st m1a2st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

kirktrue
kirktrue reviewed Sep 2, 2025
View reviewed changes
Copy link
Contributor

@kirktrue kirktrue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @AndrewJSchofield! I like the switch to dash vs. underscore for readability, though it's subjective.

Are we sticking with the term "messages" over "records?" I'm sure I'm getting mixed up, but I thought there was an effort to switch the tools over to using records.

Thanks!

@AndrewJSchofield
Copy link
Member Author

Thanks for the PR @AndrewJSchofield! I like the switch to dash vs. underscore for readability, though it's subjective.

Are we sticking with the term "messages" over "records?" I'm sure I'm getting mixed up, but I thought there was an effort to switch the tools over to using records.

Thanks!

Thanks @kirktrue. wrt to "messages" vs "records", we prefer "records" in general and are moving in that direction. The verifiable producer/consumer still use "messages" and KIP-1147 didn't include changing those to "records".

lianetm
lianetm approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@lianetm lianetm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@AndrewJSchofield AndrewJSchofield merged commit 4b9075b into apache:trunk Sep 3, 2025
28 checks passed
@AndrewJSchofield AndrewJSchofield deleted the KAFKA-19653 branch September 3, 2025 14:38
chia7712
chia7712 reviewed Sep 3, 2025
View reviewed changes
tools/src/main/java/org/apache/kafka/tools/VerifiableShareConsumer.java

parser.addArgument("--command-config")
.action(store())
.required(false)
.type(String.class)
.dest("commandConfig")
.metavar("CONFIG_FILE")
.metavar("CONFIG-FILE")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kafka currently uses two parser libraryies: joptsimple and argparse4j. As a result, not all tools support metavariables. I'm not sure why Kafka relies on both parsers, but it would be beneficial to unify them to ensure a consistent codebase and output message.

@AndrewJSchofield @lianetm @kirktrue @m1a2st WDYT?

Copy link
Collaborator

@Yunyung Yunyung Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a bit annoying. If we have to choose between the two, we should probably use joptsimple everywhere, since most scripts already rely on CommandDefaultOptions, which works well with CommandLineUtils.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree in principle, the syntax supported by kafka-producer-perf-test is a bit different. We might accidentally cause existing commands to fail. So, care needed here for sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened https://issues.apache.org/jira/browse/KAFKA-19670

Since argparse4j provides more powerful features, we could consider using it for the codebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chia7712 chia7712 chia7712 left review comments

@lianetm lianetm lianetm approved these changes

+3 more reviewers

@kirktrue kirktrue kirktrue left review comments

@Yunyung Yunyung Yunyung left review comments

@m1a2st m1a2st m1a2st approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ci-approved tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AndrewJSchofield @Yunyung @kirktrue @chia7712 @lianetm @m1a2st