Skip to content

[ISSUE #10589] Rename DecodeMessageIdCommond to DecodeMessageIdCommand#10590

Open
yx9o wants to merge 3 commits into
apache:developfrom
yx9o:dev-10589
Open

[ISSUE #10589] Rename DecodeMessageIdCommond to DecodeMessageIdCommand#10590
yx9o wants to merge 3 commits into
apache:developfrom
yx9o:dev-10589

Conversation

@yx9o

@yx9o yx9o commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

@codecov-commenter

codecov-commenter commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.25%. Comparing base (8242c1e) to head (6753e15).
⚠️ Report is 7 commits behind head on develop.

Files with missing lines Patch % Lines
.../tools/command/message/DecodeMessageIdCommand.java 89.65% 3 Missing ⚠️
.../apache/rocketmq/tools/command/MQAdminStartup.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10590      +/-   ##
=============================================
- Coverage      48.26%   48.25%   -0.01%     
- Complexity     13433    13466      +33     
=============================================
  Files           1378     1380       +2     
  Lines         100817   101022     +205     
  Branches       13040    13084      +44     
=============================================
+ Hits           48660    48753      +93     
- Misses         46211    46289      +78     
- Partials        5946     5980      +34     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RongtongJin RongtongJin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left one inline comment about the unused command.

import org.apache.rocketmq.tools.command.SubCommandException;

public class DecodeMessageIdCommond implements SubCommand {
public class DecodeMessageIdCommand implements SubCommand {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This command does not seem to be registered or referenced anywhere. Would it be better to remove it instead of renaming it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Since this command has a clear purpose but is currently not registered, I prefer keeping it and registering it in MQAdminStartup instead of removing it.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot

Summary

Fixes a typo in the class name (DecodeMessageIdCommondDecodeMessageIdCommand), registers the command in MQAdminStartup, and makes the -i option required (since decoding without a message ID is meaningless).

Findings

  • [Info] MQAdminStartup.java:265 — The old class DecodeMessageIdCommond was never registered in initCommand(), so this effectively adds a new usable command rather than renaming an existing one. No backward-compatibility concern.

  • [Info] DecodeMessageIdCommand.java:30 — Changing commandName() from "DecodeMessageId" to "decodeMessageId" follows the camelCase convention used by other subcommands. Good.

  • [Info] DecodeMessageIdCommand.java:41 — Making -i required (setRequired(true)) is correct — the command cannot function without a message ID.

Verdict

Clean, small fix. Typo correction + proper command registration. LGTM.


Automated review by github-manager-bot

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot (re-review)

Update since last review

New commit 4fe02bc (2026-07-08) addresses @RongtongJin's feedback by registering DecodeMessageIdCommand in MQAdminStartup.initCommand(). The command is now properly wired into the CLI tool.

Changes in this commit

  1. MQAdminStartup.java: Added import and initCommand(new DecodeMessageIdCommand()) — resolves the "unused command" concern
  2. commandName(): Changed from "DecodeMessageId" to "decodeMessageId" — follows camelCase convention consistent with other subcommands
  3. commandDesc(): Capitalized first letter, added period — minor style improvement
  4. -i option: Changed from setRequired(false) to setRequired(true) — prevents NPE when no message ID is provided
  5. Output format: Changed from ip=%s / date=%s to IP: %s%n / Date: %s%n — more readable with proper newlines
  6. Error handling: Consolidated two separate try-catch blocks into one, replaced silent e.printStackTrace() with SubCommandException — proper error propagation

Findings

  • [Warning] DecodeMessageIdCommand.java:31commandName() changed from "DecodeMessageId" to "decodeMessageId". This is a breaking change for users who have scripts or automation relying on the old command name (e.g., mqadmin DecodeMessageId -i xxx). Consider keeping backward compatibility by supporting both names, or at minimum document this in the PR description / release notes.

  • [Info] DecodeMessageIdCommand.java:37 — Making -i required is the right call. The old code would NPE on commandLine.getOptionValue('i').trim() if the option was omitted. Good fix.

  • [Info] DecodeMessageIdCommand.java:50-53 — Output format change (IP: ... / Date: ...) is more readable. Note that scripts parsing the old ip=.../date=... format will need updating.

Verdict

The new commit properly addresses the reviewer feedback. The code changes are clean and improve error handling. The main concern is the CLI command name change — flagging it as a potential compatibility issue for downstream users.


Automated review by github-manager-bot

String date = UtilAll.formatDate(MessageClientIDSetter.getNearlyTimeFromID(messageId), UtilAll.YYYY_MM_DD_HH_MM_SS_SSS);
System.out.printf("date=%s", date);
System.out.printf("IP: %s%n", MessageClientIDSetter.getIPStrFromID(messageId));
System.out.printf("Date: %s%n", UtilAll.formatDate(MessageClientIDSetter.getNearlyTimeFromID(messageId), UtilAll.YYYY_MM_DD_HH_MM_SS_SSS));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is recommended to perform a complete parsing of the offsetId field

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, offsetId parsing is supported now. Please review again, thanks.

@RockteMQ-AI

Copy link
Copy Markdown
Contributor

What changed

  • Added -t offset option to decode offset-format message IDs into StoreHost + CommitLogOffset
  • MessageDecoder.decodeMessageId() is used for full parsing — this correctly addresses the review suggestion
  • Tests cover unique/offset/IPv4/IPv6 scenarios and error handling

Assessment

The new commit properly resolves the review feedback. The offsetId parsing logic is clean and well-tested.

Minor observations:

  1. Command name case change: DecodeMessageIddecodeMessageId — this is a breaking change for scripts using mqadmin DecodeMessageId. If backward compatibility matters, consider keeping the original casing or documenting the rename in release notes.

  2. -i option now required: Previously optional (with interactive prompt), now marked requiredOpt. This is a behavior change — users who relied on the interactive prompt will now get an error. Consider whether this is intentional.

  3. formatStoreHost duplication: The same hex-to-InetSocketAddress logic exists in both DecodeMessageIdCommand and DecodeMessageIdCommandTest. Minor — could extract to a shared utility if desired.

Overall the implementation looks solid. 👍

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot

Summary

Renames DecodeMessageIdCommondDecodeMessageIdCommand (typo fix) and registers it as an mqadmin subcommand. Also adds offset message ID decoding support alongside the existing unique ID decoding.

Findings

  • [Info] DecodeMessageIdCommand.java:68-78 — The offset ID parsing via MessageDecoder.decodeMessageId() correctly extracts both the store host and commit log offset. This addresses the earlier review feedback about complete offsetId parsing.
  • [Info] DecodeMessageIdCommand.java:55-66 — The unique ID path uses MessageClientIDSetter.getUniqID() + MessageDecoder.decodeMsgId(), which is the standard approach. Good.
  • [Info] MQAdminStartup.java — The old class DecodeMessageIdCommond was never registered in initCommand(), so adding the new registration here is correct — no backward compatibility concern.
  • [Info] DecodeMessageIdCommandTest.java — Test coverage is solid: covers unique/offset types, IPv4/IPv6 store hosts, and the unsupported type error path.

Suggestions

  • Minor: Consider adding a brief usage example in the command name() or commandName() javadoc so users can discover the supported -t values (unique/offset) from mqadmin help decodeMessageId. Not blocking.

Verdict

Clean rename with meaningful functional improvement. Addresses prior review feedback. Tests are comprehensive.


Automated review by github-manager-bot

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Rename misspelled DecodeMessageIdCommond class

5 participants