Skip to content

MINOR: Refactor JmxTool and StreamsResetter to use CommandLineUtils#maybePrintHelpOrVersion#10

Draft
apalan60 wants to merge 8 commits into
trunkfrom
MINOR
Draft

MINOR: Refactor JmxTool and StreamsResetter to use CommandLineUtils#maybePrintHelpOrVersion#10
apalan60 wants to merge 8 commits into
trunkfrom
MINOR

Conversation

@apalan60

@apalan60 apalan60 commented Sep 3, 2025

Copy link
Copy Markdown
Owner

No description provided.

@apalan60

apalan60 commented Sep 3, 2025

Copy link
Copy Markdown
Owner Author

/review
--pr_reviewer.extra_instructions="
In the possible issues section, emphasize the following:

  • Is the code logic efficient?

"
--pr_reviewer.inline_code_comments=true
-i
--pr_reviewer.require_can_be_split_review=true
--pr_reviewer.num_code_suggestions="5"

@apalan60

apalan60 commented Sep 3, 2025

Copy link
Copy Markdown
Owner Author

/describe
--pr_description.publish_description_as_comment=true
--pr_description.generate_ai_title=true

@apalan60

apalan60 commented Sep 3, 2025

Copy link
Copy Markdown
Owner Author

/improve
--pr_code_suggestions.extra_instructions="
Emphasize the following:

  • Does the code logic cover relevant edge cases?
  • Is the code logic clear and easy to understand?
  • Is the code logic efficient?

"
--pr_code_suggestions.num_code_suggestions_per_chunk="4"
--pr_code_suggestions.commitable_code_suggestions=true
--pr_code_suggestions.suggestions_score_threshold="0"

@qodo-code-review

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
🔀 Multiple PR themes

Sub-PR theme: Refactor JmxTool to use CommandLineUtils.maybePrintHelpOrVersion

Relevant files:

  • tools/src/main/java/org/apache/kafka/tools/JmxTool.java

Sub-PR theme: Refactor StreamsResetter to use CommandLineUtils.maybePrintHelpOrVersion and validate argument checks

Relevant files:

  • tools/src/main/java/org/apache/kafka/tools/StreamsResetter.java

⚡ Recommended focus areas for review

Efficiency

The same invalid-args check is performed four times with different primary arguments; verify whether these repeated calls are required by design. If not, consider consolidating to avoid redundant work at startup.

CommandLineUtils.checkInvalidArgs(parser, options, toOffsetOption, toDatetimeOption, byDurationOption, toEarliestOption, toLatestOption, fromFileOption, shiftByOption);
CommandLineUtils.checkInvalidArgs(parser, options, toDatetimeOption, toOffsetOption, byDurationOption, toEarliestOption, toLatestOption, fromFileOption, shiftByOption);
CommandLineUtils.checkInvalidArgs(parser, options, byDurationOption, toOffsetOption, toDatetimeOption, toEarliestOption, toLatestOption, fromFileOption, shiftByOption);
CommandLineUtils.checkInvalidArgs(parser, options, toEarliestOption, toOffsetOption, toDatetimeOption, byDurationOption, toLatestOption, fromFileOption, shiftByOption);
Behavior Change

Replacing explicit help/version handling with a utility method may alter exit codes or side effects; confirm parity with previous behavior and that options conforms to the utility’s expectations.

CommandLineUtils.maybePrintHelpOrVersion(options, "Dump JMX values to standard output.");

@apalan60

apalan60 commented Sep 3, 2025

Copy link
Copy Markdown
Owner Author

/gemini review

@qodo-code-review

Copy link
Copy Markdown

Title

Refactor command-line tools to use centralized help/version handling


PR Type

Other


Description

  • Refactor help and version handling in command-line tools

  • Replace duplicate code with centralized utility method

  • Simplify conditional logic in JmxTool and StreamsResetter


Diagram Walkthrough

flowchart LR
  A["JmxTool.main()"] --> B["CommandLineUtils.maybePrintHelpOrVersion()"]
  C["StreamsResetter constructor"] --> B
  B --> D["Centralized help/version logic"]
Loading

File Walkthrough

Relevant files
Enhancement
JmxTool.java
Simplify help/version handling in JmxTool                               

tools/src/main/java/org/apache/kafka/tools/JmxTool.java

  • Replace separate help and version checks with single utility call
  • Remove 8 lines of duplicate conditional logic
  • Maintain same functionality with cleaner code
+1/-8     
StreamsResetter.java
Simplify help/version handling in StreamsResetter               

tools/src/main/java/org/apache/kafka/tools/StreamsResetter.java

  • Replace separate help and version checks with single utility call
  • Remove 6 lines of duplicate conditional logic
  • Streamline constructor initialization flow
+1/-6     

@qodo-code-review

Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors JmxTool and StreamsResetter to use the CommandLineUtils#maybePrintHelpOrVersion utility method. This change correctly simplifies the command-line argument parsing logic for handling --help and --version flags, replacing boilerplate code with a single method call. The refactoring improves code readability and maintainability by reducing duplication. The changes are well-implemented and I have no further suggestions.

Rancho-7 and others added 7 commits September 4, 2025 02:29
…sage key/header support (apache#20301)

jira: https://issues.apache.org/jira/browse/KAFKA-19011  kip:

https://cwiki.apache.org/confluence/display/KAFKA/KIP-1172%3A+Improve+EndToEndLatency+tool

This PR improves the usability and maintainability of the
`kafka-e2e-latency.sh` tool:

- Replaces fixed-index argument parsing with a proper argument parser
(joptsimple)
- Adds support for configuring:
    - -record-key-size: size of the message key
    - -num-headers: number of headers per message
    - -record-header-key-size: size of each header key
    - -record-header-size: size of each header value
- Renames existing arguments to align with Kafka CLI conventions:
    - broker_list → bootstrap-server
    - num_messages → num-records
    - message_size_bytes → record-size
    - properties_file → command-config
    -

Reviewers: Jhen-Yung Hsu <jhenyunghsu@gmail.com>, Ken Huang
 <s7133700@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
…Id (apache#20420)

Add tests for producer state listing with, without, and invalid
brokerId.

Reviewers: TengYao Chi <kitingiao@gmail.com>, Chia-Ping Tsai
 <chia7712@gmail.com>
…is too large (apache#20046)

* Log error message if `broker.heartbeat.interval.ms * 2` is large than
`broker.session.timeout.ms`.
* Add test case

`testLogBrokerHeartbeatIntervalMsShouldBeLowerThanHalfOfBrokerSessionTimeoutMs`.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
- rewrite code to avoid @Suppress
- remove unused code
- fix test error message

Reviewer: Lucas Brutschy <lbrutschy@confluent.io>
Update the supported tags for the 4.1.0 release

Reviewers: Luke Chen <showuon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants