[ https://issues.apache.org/jira/browse/HDDS-351?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16594658#comment-16594658 ]
Anu Engineer commented on HDDS-351: ----------------------------------- [~ajayydv] Thanks for providing the patch. I have some minor comments and some questions. Thank you for being patient with me. # EventQueue.java - I am afraid adding removeHandler would cause race conditions to the code since the Data structures do not seem to be protected. # nit: MiniOzoneClusterImpl.java - "Stopping down" ==> stopping # SCMChillModeManager.java: Line 72#run() - Why do we have a run function in this code path? That thread starts up and immediately dies. I don't understand why we need to run this in the context of a thread. From a quick reading, it looks like we could have done this in a constructor. # SCMChillModeManager.java: Line 75 - This implies that getScmContainerManager is finished initializing. If that is the contract that we want to support, we must make it explicit. We need to create a flag in ScmContainerManager that indicates that is done with init. We need to assert that info here and take a dependency on that; otherwise we can avoid depending on ContainerManager. # exitRules[0] - This seems like a poor coding practice. If we want to add rules that need to evaluate, I suggest that we create a list, and invoke the add function. The problem is not in this line. The problem is in line 118, where if someone changes the Array index the code will break. That kind of dependency on an index of an array and function pointer is very brittle. # private double maxContainer; why is this double? we are assigning an "int" to this later. #Line 82: validateChillModeExitRules – if we have rule framework, then hard coding if (maxContainer == 0) exitChillMode seems a violation of the framework that we are building up. # ((ContainerChillModeRule) exitRules[0]).clear(); – why? # Already commented this seems like a bad pattern of code. exitRules[0].process(nodeRegistrationContainerReport); # The model of exit criteria seems little off in my mind. I think we should have an object that takes the current state of the cluster and the expected state of the cluster – Nodes, Closed Containers, Open Containers, and Pipelines. The expected and current state will allow these *ExitRule classes to implement code that decides if we should exit from chill mode. #maxContainer – why is this variable being accessed from an inner class? Why are we not passing this as a value into the inner class? Why bind to the outer class? #SCMDatanodeProtocolServer.java: Shouldn't the {{eventPublisher.fireEvent}} be inside the if? # What is the difference between NODE_REGISTRATION_CONT_REPORT and just CONTAINER_REPORT? #StorageContainerManager.java – why remove the START_REPLICATION event? > 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 > > > 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