[
https://issues.apache.org/jira/browse/HDDS-351?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16594658#comment-16594658
]
Anu Engineer edited comment on HDDS-351 at 8/28/18 7:44 AM:
------------------------------------------------------------
[~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?
was (Author: anu):
[~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: [email protected]
For additional commands, e-mail: [email protected]
