[
https://issues.apache.org/jira/browse/HDFS-9671?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15129207#comment-15129207
]
Lei (Eddy) Xu edited comment on HDFS-9671 at 2/2/16 10:42 PM:
--------------------------------------------------------------
Hey, [~anu]
Thanks for providing this patch. Would you mind to address the following
comments?
* {{WorkItem.java}} misses apache license.
* Could you rename {{WorkItem}} and {{WorkStatus}} to {{DiskBalancerWorkItem}}
and {{DiskBalancerWorkStatus}}. They are in the "o.a.h.h.s.datanode" namespace,
while the names are too general.
* IMO, {{private long copiedSoFar;}} might be named to {{private long
bytesCopied}}. Putting units into the variable makes it little bit easier for
me to understand.
* Would you mind to fix comments to follow jdoc format? e.g., we do not need
{{-}} in {{@param errMsg - msg}}. Also there are a few places like {{@return
long}}, that we can probably remove.
* {{DiskbalancerException}} should be {{DiskBalancerException}}.
* Are {{DiskBalancerException.DISK_BALANCE_NOT_ENABLED, INVALID_PLAN, ...}} the
only possible value for {{int result}}? Could us use a {{enum}} here?j
* {code}
try {
lock.lock();
{code}
We might want to move {{lock()}} out of {{try \{..\}}}
* {{BlockMover}} should hold a {{FsVolumeReference}} when doing the IOs. So in
this sense, it might extend from {{Closable}}.
* {code}
try {
312 synchronized (this.dataset) {
313 references = this.dataset.getFsVolumeReferences();
....
references.close();
{code}
You can probably directly use JDK7 {{try-ressource}} to manage {{references}}.
* A few logs like {{LOG.info("Disk Balancer - Unable to find destination
volume. " ...}} should use {{LOG.error()}}?
* {{getPathToVolumeMap()}} is misleading. The key is {{storageID}} but not a
path.
* {{LOG.info("Executing Disk balancer plan ");}} would you mind to also output
plan ID in the log?
* Either in {{VolumePair(source, target)}} or {{createWorkPlan}}, you might
want to check that {{soruce}} dose not equal to {{dest}}?
Again, thanks for the work, [~anu]!
was (Author: eddyxu):
Hey, [~anu]
Thanks for providing this patch. Would you mind to address the following
patches?
* {{WorkItem.java}} misses apache license.
* Could you rename {{WorkItem}} and {{WorkStatus}} to {{DiskBalancerWorkItem}}
and {{DiskBalancerWorkStatus}}. They are in the "o.a.h.h.s.datanode" namespace,
while the names are too general.
* IMO, {{private long copiedSoFar;}} might be named to {{private long
bytesCopied}}. Putting units into the variable makes it little bit easier for
me to understand.
* Would you mind to fix comments to follow jdoc format? e.g., we do not need
{{-}} in {{@param errMsg - msg}}. Also there are a few places like {{@return
long}}, that we can probably remove.
* {{DiskbalancerException}} should be {{DiskBalancerException}}.
* Are {{DiskBalancerException.DISK_BALANCE_NOT_ENABLED, INVALID_PLAN, ...}} the
only possible value for {{int result}}? Could us use a {{enum}} here?j
* {code}
try {
lock.lock();
{code}
We might want to move {{lock()}} out of {{try \{..\}}}
* {{BlockMover}} should hold a {{FsVolumeReference}} when doing the IOs. So in
this sense, it might extend from {{Closable}}.
* {code}
try {
312 synchronized (this.dataset) {
313 references = this.dataset.getFsVolumeReferences();
....
references.close();
{code}
You can probably directly use JDK7 {{try-ressource}} to manage {{references}}.
* A few logs like {{LOG.info("Disk Balancer - Unable to find destination
volume. " ...}} should use {{LOG.error()}}?
* {{getPathToVolumeMap()}} is misleading. The key is {{storageID}} but not a
path.
* {{LOG.info("Executing Disk balancer plan ");}} would you mind to also output
plan ID in the log?
* Either in {{VolumePair(source, target)}} or {{createWorkPlan}}, you might
want to check that {{soruce}} dose not equal to {{dest}}?
Again, thanks for the work, [~anu]!
> DiskBalancer : SubmitPlan implementation
> -----------------------------------------
>
> Key: HDFS-9671
> URL: https://issues.apache.org/jira/browse/HDFS-9671
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: balancer & mover
> Affects Versions: HDFS-1312
> Reporter: Anu Engineer
> Assignee: Anu Engineer
> Attachments: HDFS-9671-HDFS-1312.001.patch,
> HDFS-9671-HDFS-1312.002.patch
>
>
> Datanode side code for submit plan for diskbalancer.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
