[
https://issues.apache.org/jira/browse/HDDS-351?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16597728#comment-16597728
]
Ajay Kumar edited comment on HDDS-351 at 8/30/18 6:02 PM:
----------------------------------------------------------
[~elek] thanks for review.
{quote}I don't understand why do we need a Map for exitRules if we use just the
only one element everywhere. I think one single variable should be enough. Or I
would expect at least some loops.
For example why don't we cleanup other exitRules just
exitRules.get(CONT_EXIT_RULE).cleanup()?
And why do we need the CONT_EXIT_RULE? Are the keys of exitRules used
somewhere? (Maybe I missed something...){quote}
To make it more generic to add more rules down the line. CONT_EXIT_RULE is
added to avoid hardcoding string and increase readability.
{quote}Until know we tried to follow a practice where all the event wiring are
defined in StorageContainerManager. The only one exception was the EventWatcher
but we also discussed it with Lokesh that the wiring logic could be moved out
from the EventWatcher constructor. To be honest I have no preference. The only
thing what I would like to do is handle all the subscription logic with the
same way. Either wire everything in the SCM class or move all the
eventPublisher subscription logic to constructors.{quote}
Moved wiring to SCM constructor in patch v4.
{quote}Would be great to write a unit test for ContainerChillModeRule. I am not
sure, but IMHO containerWithMinReplicas should be incremented only if the
containerReplicaMap.get(c.getContainerID()) was empty. berfore. A unit test
would convince me...
{quote}
Removed containerReplicaMap to simplify the patch. Test case in TestSCM tests
exit logic.
{quote}Not clear why do you do containers.remove(c.getContainerID()); at
SCMChillModeManager:L171{quote}
We want to track only containers whose replica is not reported yet.
{quote}Why did you remove: eventQueue.addHandler(SCMEvents.START_REPLICATION,
replicationStatus); from SCM{quote}
that seems like a mistake, mistook it as emitting of START_REPLICATION event.
Reverted this in new patch.
was (Author: ajayydv):
[~elek] thanks for review.
{quote}I don't understand why do we need a Map for exitRules if we use just the
only one element everywhere. I think one single variable should be enough. Or I
would expect at least some loops.
For example why don't we cleanup other exitRules just
exitRules.get(CONT_EXIT_RULE).cleanup()?
And why do we need the CONT_EXIT_RULE? Are the keys of exitRules used
somewhere? (Maybe I missed something...){quote}
To make it more generic to add more rules down the line. CONT_EXIT_RULE is
added to avoid hardcoding string and increase readability.
{quote}Until know we tried to follow a practice where all the event wiring are
defined in StorageContainerManager. The only one exception was the EventWatcher
but we also discussed it with Lokesh that the wiring logic could be moved out
from the EventWatcher constructor. To be honest I have no preference. The only
thing what I would like to do is handle all the subscription logic with the
same way. Either wire everything in the SCM class or move all the
eventPublisher subscription logic to constructors.{quote}
Moved wiring to SCM constructor in patch v4.
{quote}Would be great to write a unit test for ContainerChillModeRule. I am not
sure, but IMHO containerWithMinReplicas should be incremented only if the
containerReplicaMap.get(c.getContainerID()) was empty. berfore. A unit test
would convince me...
{quote}
Removed containerReplicaMap to simplify the patch. Test case in TestSCM tests
exit logic.
{quote}Not clear why do you do containers.remove(c.getContainerID()); at
SCMChillModeManager:L171{quote}
We want to track only containers whose replica is not reported yet.
{quote}Why did you remove: eventQueue.addHandler(SCMEvents.START_REPLICATION,
replicationStatus); from SCM{quote}
that seems like a mistake, mistook it as emitting of 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, 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: [email protected]
For additional commands, e-mail: [email protected]
