[ 
https://issues.apache.org/jira/browse/HDFS-7411?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14295795#comment-14295795
 ] 

Tsz Wo Nicholas Sze commented on HDFS-7411:
-------------------------------------------

> Those methods you mentioned are pretty small, and are far from the bulk of 
> the patch. ...

The GenericHdfsTestUtils change also adds a significant amount of code to the 
patch.  The code refactor does seem occupying half of the patch.

> ... Two other reviewers have also made it through this successfully patch, so 
> I don't think it's so bad to review.

I did not say that it is impossible to review the patch.  It just unnecessarily 
complicated so that reviewers need to spend extra time to review the patch.

> Regarding the removed config property, this is something discussed above. ...

It seems the discussion above did not consider the incompatibility.  I guess 
the unnecessarily complicated and large patch did hide the important details.  
We need to revisit it.

> ... I don't see a way of deprecating this gracefully, since the units of the 
> old and new config properties are incompatible. ...

One way is to keep the original code.  Use the old code if the old conf is set 
and the new code if new conf is set.  Isn't it simple enough?

Since this involve an incompatible change, please move the refactoring code out 
from the improvement.

> Refactor and improve decommissioning logic into DecommissionManager
> -------------------------------------------------------------------
>
>                 Key: HDFS-7411
>                 URL: https://issues.apache.org/jira/browse/HDFS-7411
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>    Affects Versions: 2.5.1
>            Reporter: Andrew Wang
>            Assignee: Andrew Wang
>         Attachments: hdfs-7411.001.patch, hdfs-7411.002.patch, 
> hdfs-7411.003.patch, hdfs-7411.004.patch, hdfs-7411.005.patch, 
> hdfs-7411.006.patch, hdfs-7411.007.patch, hdfs-7411.008.patch, 
> hdfs-7411.009.patch
>
>
> Would be nice to split out decommission logic from DatanodeManager to 
> DecommissionManager.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to