Skip to content
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.

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

HDDS-11878. Use CommandSpec to find top-level command #7575

Merged
merged 8 commits into from
Dec 26, 2024

Conversation

adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Dec 13, 2024

What changes were proposed in this pull request?

Problem

GenericParentCommand provides a way to access GenericCli methods from sub-subcommands. However, its usage is tedious and comes with boilerplate code that needs to:

  • implement GenericParentCommand
  • define methods manually
  • define member variable for @ParentCommand (injected by picocli)

Adding new methods to GenericParentCommand becomes harder the more implementations we have. The interface needs to be implemented on all sub-command levels.
Currently only very few implementations exists. Some subcommands access top-level command via similar parent chain, but without implementing GenericParentCommand.

Solution

Picocli keeps track of the command hierarchy and allows accessing the top-level command directly via CommandSpec.

Changes in this PR:

  • Introduce new AbstractSubcommand/AbstractMixin base classes. Subcommands/mixins that want to access the top-level command can do so easily by extending it.
  • Remove boilerplate implementation of GenericParentCommand.
  • Remove parent chains where it is only used to access the top-level command.

Accessing the top-level command directly makes it easier to reorganize subcommands, add them to different parents, or even multiple parents at different levels.

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

How was this patch tested?

Tested some of the commands changed, that they still reflect the --verbose flag:

$ ozone sh --verbose volume info /vol1
Volume Name : vol1
VOLUME_NOT_FOUND org.apache.hadoop.ozone.om.exceptions.OMException: Volume vol1 is not found
	at org.apache.hadoop.ozone.om.protocolPB.OzoneManagerProtocolClientSideTranslatorPB.handleError(OzoneManagerProtocolClientSideTranslatorPB.java:763)
	at org.apache.hadoop.ozone.om.protocolPB.OzoneManagerProtocolClientSideTranslatorPB.getVolumeInfo(OzoneManagerProtocolClientSideTranslatorPB.java:462)
	at org.apache.hadoop.ozone.client.rpc.RpcClient.getVolumeDetails(RpcClient.java:498)
$ ozone admin container info 1
Container id: 1
Pipeline id: 9dc28c7f-4e45-4a3b-8d87-ce8b1d788e86
...

$ ozone admin --verbose container info 1
Container id: 1
Pipeline Info: Pipeline[ Id: 9dc28c7f-4e45-4a3b-8d87-ce8b1d788e86, Nodes: 5b43e396-c1e8-4ab0-907a-1988d8ae3ff7(ozone-datanode-1.ozone_default/172.17.0.8) ReplicaIndex: 0f40dd8b8-bf0d-4e71-b3b7-6f99cedbf786(ozone-datanode-3.ozone_default/172.17.0.9) ReplicaIndex: 0d930ec97-4ffd-4d49-8177-96430fc508ae(ozone-datanode-2.ozone_default/172.17.0.2) ReplicaIndex: 0, ReplicationConfig: RATIS/THREE, State:OPEN, leaderId:5b43e396-c1e8-4ab0-907a-1988d8ae3ff7, CreationTimestamp2024-12-13T21:28:06.443Z[UTC]]
...

CI:
https://github.com/adoroszlai/ozone/actions/runs/12428823594

@adoroszlai adoroszlai added the tools Tools that helps with debugging label Dec 13, 2024
@adoroszlai adoroszlai self-assigned this Dec 13, 2024
@adoroszlai adoroszlai added the code-cleanup Changes that aim to make code better, without changing functionality. label Dec 19, 2024
@adoroszlai adoroszlai marked this pull request as draft December 20, 2024 07:45
@adoroszlai adoroszlai marked this pull request as ready for review December 20, 2024 10:24
@adoroszlai
Copy link
Contributor Author

@nandakumar131 please review. This is based on your comment about removing GenericParentCommand.

@nandakumar131
Copy link
Contributor

Thanks @adoroszlai for working on this.
At high level the change looks good to me, I will take a closer look at this tomorrow.

@nandakumar131
Copy link
Contributor

Not related to this change:
While reviewing the code I realised that there are two ways in which we are handling MissingSubCommand.

throw new MissingSubcommandException(
        this.shell.getCmd().getSubcommands().get("xxx"));

and

GenericCli.missingSubcommand(spec);

Can we unify this as well? I can take it up.

Copy link
Contributor

@nandakumar131 nandakumar131 left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -105,7 +105,6 @@ public Void call() throws Exception {
throw new MissingSubcommandException(cmd);
}

@Override
public OzoneConfiguration createOzoneConfiguration() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be made private and only expose getOzoneConf to avoid creating multiple OzoneConfiguration. We can do this in a follow-up jira.

Copy link
Contributor Author

@adoroszlai adoroszlai Dec 26, 2024

Choose a reason for hiding this comment

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

Created HDDS-11992 for this, please feel free to pick up, if you have time, otherwise I can work on it.

@nandakumar131 nandakumar131 merged commit f125363 into apache:master Dec 26, 2024
55 checks passed
@nandakumar131
Copy link
Contributor

Thanks @adoroszlai for the contribution!

@adoroszlai adoroszlai deleted the HDDS-11878 branch December 26, 2024 08:56
@adoroszlai
Copy link
Contributor Author

Thanks @nandakumar131 for reviewing and merging this.

Not related to this change: While reviewing the code I realised that there are two ways in which we are handling MissingSubCommand.
...
Can we unify this as well? I can take it up.

Good point, but actually neither of these are necessary with the current version of picocli. This will be cleaned up in HDDS-11880.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-cleanup Changes that aim to make code better, without changing functionality. tools Tools that helps with debugging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants