You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/13 01:32:53 UTC
[GitHub] [ozone] mukul1987 opened a new pull request, #3306: HDDS-6579. Add commandline argument for Container Info command.
mukul1987 opened a new pull request, #3306:
URL: https://github.com/apache/ozone/pull/3306
## What changes were proposed in this pull request?
Adds a command line option called --addReplicaDetails to print replica related information for container info command
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6579
## How was this patch tested?
Fixed an existing test
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mukul1987 commented on pull request #3306: HDDS-6579. Add commandline argument for Container Info command.
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on PR #3306:
URL: https://github.com/apache/ozone/pull/3306#issuecomment-1098073293
> Thanks @mukul1987 for updating the patch.
>
> I just noticed that for `--json` output replicas are always included, regardless of `--replicas` flag. Is that OK?
i think that should be ok, as json output can be verbose.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mukul1987 merged pull request #3306: HDDS-6579. Add commandline argument for Container Info command.
Posted by GitBox <gi...@apache.org>.
mukul1987 merged PR #3306:
URL: https://github.com/apache/ozone/pull/3306
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3306: HDDS-6579. Add commandline argument for Container Info command.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on PR #3306:
URL: https://github.com/apache/ozone/pull/3306#issuecomment-1103032556
In my opinion this change needs reverted and rethought. When we have 10 such changes to this **human readable** command output, and someone needs all the information are they supposed to pass something like the below?
```
ozone admin container list --a --b --c --d --e --f --g --h --i --j --k ....
```
The user friendly approach is to display all the output. In order to be compatible, we don't guarantee the total output will never change - more we should guarantee information will never be removed, but more information can be added. Otherwise we are overly constraining ourselves for the future. In this case the new information is in a new section of the report and does not alter any existing information. It meets the criteria of "adding and not modifying existing code".
For these "free text" CLI commands. which are generally intended for an administrator to read and use to debug problems, I also believe we should not even be concerned about the formatting of the existing information changing, but I am open to discuss the pros and cons of that. I can see cases where we may want to add some additional information in an existing line of output as the product matures and features are added.
Most commands provide a JSON output too, and it is designed to be consumed by a computer rather than a human. If someone develops a tool to consume this data, they should be consuming the JSON data, which has a well defined parsing approach, and is suited to addition of new fields and data. If the program consuming it is correctly coded, it will not be impacted by new additions, provided existing fields do not move location and are not renamed.
This approach here is not scalable as we simply cannot add flag after flag to for each new piece of information we want to add to each command.
For the record, I am -1 on this change, and I would like it reverted and a discussion in the community to agree a way forward if my arguments here are not sufficient to convince you this is a bad path to continue down.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on pull request #3306: HDDS-6579. Add commandline argument for Container Info command.
Posted by GitBox <gi...@apache.org>.
fapifta commented on PR #3306:
URL: https://github.com/apache/ozone/pull/3306#issuecomment-1105104431
I agree with Stephen, and I would also like to have this change to be reverted.
container info command is an administrative command, which should by default contain as much information as we can provide, and it should not require CLI options to be added to give one the full picture... This is the comfortable way, as when a human wants to check something it should have all the information in an easy to understand format. What is easy to understand can be a subject for a debate (and the current format is not the best I think), but not presenting an information renders that information to be invisible instead of easy to understand.
If we seem it useful to restrict the amount of information provided, it is better to add options for restricting the amount of information shown about a container, though I don't think there is too much use for that at the moment, as the amount of information about 1 container is not that long so far...
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a diff in pull request #3306: HDDS-6579. Add commandline argument for Container Info command.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on code in PR #3306:
URL: https://github.com/apache/ozone/pull/3306#discussion_r849132014
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/container/InfoSubcommand.java:
##########
@@ -63,6 +63,11 @@ public class InfoSubcommand extends ScmSubcommand {
description = "Format output as JSON")
private boolean json;
+ @CommandLine.Option(names = { "--addReplicaDetails" },
Review Comment:
```suggestion
@CommandLine.Option(names = { "--replicas" },
```
##########
hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestInfoSubCommand.java:
##########
@@ -85,7 +85,7 @@ public void testReplicasIncludedInOutput() throws Exception {
.thenReturn(getReplicas());
cmd = new InfoSubcommand();
CommandLine c = new CommandLine(cmd);
- c.parseArgs("1");
+ c.parseArgs("1", "--addReplicaDetails");
Review Comment:
```suggestion
c.parseArgs("1", "--replicas");
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org