errose28 commented on PR #8282:
URL: https://github.com/apache/ozone/pull/8282#issuecomment-2819431215
Thanks for working on this @sreejasahithi I have some high level comments
about the CLI for this tool. Only some of them are directly related to this
patch. I would recommend defining the whole plan for the CLI in a comment in
HDDS-12579 so that PRs can just focus on implementation.
- I think `ozone debug container` is too generic of a name. The command
should indicate that it only operates on container log files.
- I'm thinking we may want a separate subcommand for log parsing commands,
including this audit log parser, and any other parsing tools we add in the
future. Something like `ozone debug logs` or `ozone debug log-parse`.
- IMO grouping log parsing commands together makes sense because they are
not tied to a specific component and require logs to be extracted first.
- I propose `ozone debug logs container` for this command. We can later
move audit log parser to `ozone debug logs audit` and add subcommands for other
log parsers we may add based on log type.
- The parsing command can then just be `ozone debug logs container
parse`.
- The command needs an option for where to create create the DB, and how to
use it for subsequent commands. I'm not following how this works in the current
implementation.
- The Properties file should be loaded automatically without the user
needing to add it to the classpath. With this requirement, would it not be
better to just put those constants in a regular Java class?
Also here are some best practices when adding CLIs. We should update these
changes to follow these, and it would be helpful if other reviewers could also
keep an eye out for these things too:
- All CLI commands and options should be kebab case.
- All PicoCLI `description` fields need to have robust usage instructions,
since these are used to generate help messages.
- For example [this
description](https://github.com/apache/ozone/blob/da95a7e4f89b3b79da24d115001ef15cbd1bc74c/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/container/ContainerLogController.java#L34)
needs a lot more info to tell an end user what this tool actually does.
- All output for tools should go to stdout or stderr. Don't depend on log4j
since it is a toss-up how the client side is configured across clusters and the
output is often lost. It is ok for logging stack traces, but make sure that a
message shows up in stderr in this case too.
- All fatal errors need to cause the command to exit non-zero. This is
usually done by propogating an exception and letting picocli filter out the
stack trace and just print the resulting message and set the error code.
- Cases like
[this](https://github.com/apache/ozone/blob/f24f8138294d38ead4bdc07d4f35bd2f5bb86846/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/container/ListContainers.java#L55)
for example will print an error, but the command will still exit 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
