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

Xiaoyu Yao commented on HDFS-10897:
-----------------------------------

Thanks [~anu] for working on this and post the patch. I took a look at the 
patch and it looks good to me overall. 
Below are my early feedback on the code. I will post my comments on unit tests 
later.
1. NodeManager.java
  NIT: I see we have registerDataeNode/removeDataNode and 
getNodeCount/getAllNodes().
  Can we have a consistent name, e.g., all with/wo <Data>.  
  
2. SCMNodeManager.java
typo in comments:  stale informatio. 

Can we use the wrapper from HadoopExecutors?
{code}
  executorService = Executors.newScheduledThreadPool(1,
 131            new ThreadFactoryBuilder().setDaemon(true)
 132                .setNameFormat("SCM Heartbeat Processing Thread - 
%d").build());
{code}

updateHeartbeat() Can you add comments to the synchronization assumption from 
the caller if any
similar to registerDatanode()?

removeDatanode() : empty overwrite implementation. Either add a TODO: or remove
it if it is not needed.

getNodes() : declare "throws IllegalArgumentException"

> Ozone: SCM: Add NodeManager
> ---------------------------
>
>                 Key: HDFS-10897
>                 URL: https://issues.apache.org/jira/browse/HDFS-10897
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>    Affects Versions: HDFS-7240
>            Reporter: Anu Engineer
>            Assignee: Anu Engineer
>         Attachments: HDFS-10897-HDFS-7240.001.patch, 
> HDFS-10897-HDFS-7240.002.patch
>
>
> Add a nodeManager class that will be used by Storage Controller Manager 
> eventually.



--
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

Reply via email to