Skip to content

Conversation

@sreejasahithi
Copy link
Contributor

@sreejasahithi sreejasahithi commented Jun 21, 2025

What changes were proposed in this pull request?

Introduced a reusable PicoCLI mixin (NodeSelectionMixin) to standardize datanode selection options across commands. This includes:

--node-id for datanode UUID

--hostname for datanode host name

--ip for datanode IP address

Deprecated older/ambiguous options like --id, --uuid, and --address for clarity and consistency, while retaining them in hidden mode to preserve backward compatibility.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13098

How was this patch tested?

https://github.com/sreejasahithi/ozone/actions/runs/15778605382

@sreejasahithi sreejasahithi marked this pull request as ready for review June 21, 2025 14:06
Copy link
Contributor

@aryangupta1998 aryangupta1998 left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me!
If user enters a current value and a deprecated value like,
ozone admin datanode usageinfo --node-id X --uuid Y
then we should be able to warn users!

@sreejasahithi
Copy link
Contributor Author

If user enters a current value and a deprecated value like, ozone admin datanode usageinfo --node-id X --uuid Y then we should be able to warn users!

The options --node-id and --uuid are mutually exclusive, meaning only one of them can be specified at a time.
If a user provides both options, as in the following command:
ozone admin datanode usageinfo --node-id X --uuid Y

They will receive the following error message:
Error: --node-id=, --uuid= are mutually exclusive (specify only one)
This error is followed by the usage/help message.

Copy link
Contributor

@sarvekshayr sarvekshayr 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 working on this @sreejasahithi.

In TestDecommissionStatusSubCommand, the current tests cover the deprecated --id option, but there's no test for the --node-id option.
Could you please add a test case to cover --node-id as well?

Copy link
Contributor

@Tejaskriya Tejaskriya 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 working on this @sreejasahithi , please find my comments below

Also I think you can try updating your branch with the latest master, the test failures seem unrelated to your changes, it may get resolved.

@sreejasahithi sreejasahithi requested a review from Tejaskriya July 8, 2025 06:48
Copy link
Contributor

@aryangupta1998 aryangupta1998 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Tejaskriya Tejaskriya merged commit bca8688 into apache:master Jul 8, 2025
41 checks passed
@Tejaskriya
Copy link
Contributor

Thanks for the patch @sreejasahithi , and for the reviews @sarvekshayr @aryangupta1998

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jul 31, 2025
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.

4 participants