[ https://issues.apache.org/jira/browse/HDFS-10551?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15344941#comment-15344941 ]
Mingliang Liu commented on HDFS-10551: -------------------------------------- Thanks [~eddyxu]] for reporting this and [~anu] for the patch. The v2 patch looks overall good to me. Some minor comments: # In {{Command}} constructor, {{// These arguments are valid for all commands.}} can be deleted. # In {{TestDiskBalancerCommand}} other test cases can also use the class filed {{conf}} (e.g. {{testReadClusterFromJson()}}). This will also address the checkstyle warning I think. # For the config key name {{dfs.disk.balancer.max.disk.throughputInMBperSec}}, I'd suggest {{dfs.disk.balancer.max.disk.bandwidthPerSec}}. I think _throughput_ and _bandwidth_ have different meanings in different context and bandwidth here is better. This also conforms to existing names like {{dfs.image.transfer.bandwidthPerSec}}, {{dfs.balance.bandwidthPerSec}} and {{dfs.datanode.balance.bandwidthPerSec}}, to name a few. > o.a.h.h.s.diskbalancer.command.Command does not actually verify options as > expected. > ------------------------------------------------------------------------------------ > > Key: HDFS-10551 > URL: https://issues.apache.org/jira/browse/HDFS-10551 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: datanode > Reporter: Lei (Eddy) Xu > Assignee: Anu Engineer > Priority: Critical > Attachments: HDFS-10551-HDFS-1312.001.patch, > HDFS-10551-HDFS-1312.002.patch, HDFS-10551-HDFS-1312.003.patch > > > In {{diskbalancer.command.Command#verifyCommandOptions}}. The following code > does not do what it expected to do: > {code} > if (!validArgs.containsKey(opt.getArgName())) { > {code} > opt.getArgName() always returns "arg" instead of i.e., {{report}} or {{uri}}, > which is the expected parameter to check. > It should use {{opt.getLongOpt()}} to get the option names. It can pass on > the branch because {{opt.getArgName()}} always returns {{"arg"}}, which is > accidently in {{validArgs}}. However I don't think it is the intention for > this function. > Additionally, in the following code > {code} > validArguments.append("Valid arguments are : %n"); > {code} > This {{%n}} is not used. -- This message was sent by Atlassian JIRA (v6.3.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org