Mailing-List: contact hdfs-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Date: Fri, 21 Apr 2017 17:34:04 +0000 (UTC)
From: "Xiaoyu Yao (JIRA)" <jira@apache.org>
To: hdfs-issues@hadoop.apache.org
Message-ID: <JIRA.13047918.1488519932000.20706.1492796044348@Atlassian.JIRA>
In-Reply-To: <JIRA.13047918.1488519932000@Atlassian.JIRA>
References: <JIRA.13047918.1488519932000@Atlassian.JIRA> <JIRA.13047918.1488519932868@jira-lw-us.apache.org>
Subject: [jira] [Commented] (HDFS-11493) Ozone: SCM:  Add the ability to
 handle container reports
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Fri, 21 Apr 2017 17:34:10 -0000


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

Xiaoyu Yao commented on HDFS-11493:
-----------------------------------

Thanks [~anu] for working on this. The patch looks good to me. Below are some initial comments on the production code. I will post my review on the unit test later.

OzoneConfigKeys.java
NIT: Line 91 can we rename "ozone.scm.container.report.processing.lag.seconds" to "ozone.scm.container.report.processing.interval.seconds"
NIT: Line 99: can we rename "ozone.scm.max.wait.for.container.reports.in.seconds" to "ozone.scm.container.reports.wait.timeout.seconds"

CommandQueue.java
NIT: Line 135 "command" -> Commands

NodePoolManager.java
NIT: unnecessary empty line change

PeriodicPool.java
Line 26: should we use AtomicLong for totalProcessedCount?
Line 92: miss javadoc for getLastProcessTime

ContainerReplicationManager.java
Line 87: NIT: move before line 79?

Line 119: we should ensure the executorService is shutdown properly. 
Line 121: extra leading space in the name string " Container Reports..." 
Line 135: NIT: need add an empty line
Line 145: can you elaborate on the two cases?

Line 190: computerPoolDifference assumes the newPool is made a copy in the method and the oldPool is made a copy by the caller
This can be optimized with Java8 or Guava Sets.difference

Line 208: PerodicPool#lastProcessTime is never changed with this patch. Can you
add TODO: if this will be changed in later patches.

Line 243: we restart the poolProcessThread if UncaughtException is hit. Could this cause
infinite loop in certain cases?

ReplicationDatanodeStateManager.java
Line 53: missing java doc for some parameters
Line 66: can we define some SCM specific exception here?
Line 70: Random r can be moved to member to avoid new Random() for each
getContainerReport call.

> Ozone: SCM:  Add the ability to handle container reports 
> ---------------------------------------------------------
>
>                 Key: HDFS-11493
>                 URL: https://issues.apache.org/jira/browse/HDFS-11493
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>    Affects Versions: HDFS-7240
>            Reporter: Anu Engineer
>            Assignee: Anu Engineer
>         Attachments: container-replication-storage.pdf, HDFS-11493-HDFS-7240.001.patch
>
>
> Once a datanode sends the container report it is SCM's responsibility to determine if the replication levels are acceptable. If it is not, SCM should initiate a replication request to another datanode. This JIRA tracks how SCM  handles a container report.


--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org