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

Xiaoyu Yao commented on HDDS-351:
---------------------------------

Thanks [~ajayydv] for working on this. The patch looks good to me overall. A 
few more comments:

 

ContainerStateManger.java

Line 512: this changes to return an empty set instead of throwing exception. 
From the two callers, I see ContainerManpping#getContainerWithPipeline has been 
updated to throw when an empty set is returned.

But ContainerReportHandler#emitReplicationRequestEvent() is not. In the case of 
an empty set is returned, I think we should throw exception as well here as 
there is nothing to replicate here.

 

HddsConfigKeys.java

Line 67: NIT: suggest rename to HDDS_SCM_CHILLMODE_THRESHOLD_PCT 
"hdds.scm.chillmod.threshold.pct"

Line 71: NIT: suggest rename HDDS_SCM_CHILLMODE_THRESHOLD_PCT_DEFAULT

 

Ozone-default.xml

Line 1110: the tag should be "HDDS, SCM, OPERATION"

 

SCMChillModeManager.java

Line 19-20: NIT: seems the license header are accidentally changed with extra 
line changes.

Line 58: unused variable can be removed

Line 61: can we define containerWithMinReplicas as AtomicLong and use 
AtomicLong#doubleValue()  when doing the calculation in validate() at line 160.

Line 59/69: we don't need to maintain a local copy of EventQueue. 
onMessage->validateChillModeExitRules->exitChillMode() can pass the unused 
EventPublisher.

 

Line 71-72, 146-150: this can be optimized to reduce unnecessary map-list-map 
conversion. ContainerStateManager#getContainerMap can return a map directly.

Also, can you clarify if this rule expects close container only from 
containerStateManager?

 

Line 103: seems only CONT_EXIT_RULE get to process the registration report? But 
all the other rules are invoked to validate. Can you clarify with the expected 
usage for the ChillModeExitRule interface?

 ** 

TestStorageContainerManager.java

Line 535: can we add some validation on the chill mode exit state after DN 
starts?

Can we add more unit tests on the exit rules, e.g., the default percentage rule?

> Add chill mode state to SCM
> ---------------------------
>
>                 Key: HDDS-351
>                 URL: https://issues.apache.org/jira/browse/HDDS-351
>             Project: Hadoop Distributed Data Store
>          Issue Type: Bug
>            Reporter: Ajay Kumar
>            Assignee: Ajay Kumar
>            Priority: Major
>             Fix For: 0.2.1
>
>         Attachments: HDDS-351.00.patch, HDDS-351.01.patch, HDDS-351.02.patch, 
> HDDS-351.03.patch, HDDS-351.04.patch
>
>
> Add chill mode state to SCM



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

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