[
https://issues.apache.org/jira/browse/HDDS-234?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16540994#comment-16540994
]
Ajay Kumar edited comment on HDDS-234 at 7/12/18 1:36 AM:
----------------------------------------------------------
[~anu] thanks for review.
{quote}NodeReportHandler:java:
public NodeReportHandler(SCMNodeManager nm) ==> Spurious edit?{quote}
This was intentional as processNodeReport function was not included in parent
class. Didn't wanted to touch all other Mock/Test class implementing
NodeManager but on a second look i think it make sense to move it up in
NodeManager and provide empty implementation in mock classes. With that done
current patch doesn't need those changes in constructor and corresponding type
casting during initialization in StorageContainerManager.
{quote}Also you have lost the class comments for this class.
Line 33: Why lose final?{quote}
Unintended, fixed!!
{quote} It might be a good idea to write a Precondition.CheckNotNull for
NodeManager.{quote}
Done.
{quote}onMessage()*: Precondition.CheckNotNull(nodeReport); since you are
accessing members in the call. It is obvious. It might be a good idea to write
a Precondition.CheckNotNull for NodeManager.{quote}
Done
{quote}StorageContainerManager.java:156:
You have added a new field, nodeReportHandler, however you seem to assign to a
local variable in the code.{quote}
Some of these changes were not there in trunk when initial patch was uploaded,
reverted new changes in current patch.
was (Author: ajayydv):
[~anu] thanks for review.
{quote}NodeReportHandler:java:
public NodeReportHandler(SCMNodeManager nm) ==> Spurious edit?{quote}
This was intentional as processNodeReport function was not included in parent
class. Didn't wanted to touch all other Mock/Test class implementing
NodeManager but on a second look i think it make sense to move it up in
NodeManager and provide empty implementation in mock classes. With that done
current patch doesn't need those changes in constructor and corresponding type
casting during initialization in StorageContainerManager.
{quote}Also you have lost the class comments for this class.
Line 33: Why lose final?{quote}
Unintended, fixed!!
{quote} It might be a good idea to write a Precondition.CheckNotNull for
NodeManager.{quote}
Done.
{quote}onMessage()*: Precondition.CheckNotNull(nodeReport); since you are
accessing members in the call. It is obvious. It might be a good idea to write
a Precondition.CheckNotNull for NodeManager.{quote}
Done
{quote}StorageContainerManager.java:156:
You have added a new field, nodeReportHandler, however you seem to assign to a
local variable in the code.{quote}
Some of these changes were not there in trunk when initial patch was uploaded,
fixed it in current patch.
> Add SCM node report handler
> ---------------------------
>
> Key: HDDS-234
> URL: https://issues.apache.org/jira/browse/HDDS-234
> Project: Hadoop Distributed Data Store
> Issue Type: Bug
> Components: SCM
> Reporter: Ajay Kumar
> Assignee: Ajay Kumar
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-234.00.patch, HDDS-234.01.patch, HDDS-234.02.patch
>
>
> This ticket is opened to add SCM nodereport handler after the refactoring.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
