[
https://issues.apache.org/jira/browse/HDDS-234?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16540940#comment-16540940
]
Anu Engineer edited comment on HDDS-234 at 7/12/18 12:44 AM:
-------------------------------------------------------------
Thanks for taking care of this issue. Some very minor comments on the patch.
*NodeReportHandler:java*:
* public NodeReportHandler(SCMNodeManager nm) ==> Spurious edit?
* Also you have lost the class comments for this class.
* Line 33: Why lose final? It might be a good idea to write a
Precondition.CheckNotNull for NodeManager.
* onMessage()*:
Precondition.CheckNotNull(nodeReport); since you are accessing members in the
call. It is obvious. But we have bugs it is easier to debug with asserts, and
please move both Preconditions to the start of the function.
*StorageContainerManager.java*:156:
You have added a new field, nodeReportHandler, however you seem to assign to a
local variable in the code.
*Line 156*:
{code:java}
/*
* Report Handlers
* */
private NodeReportHandler nodeReportHandler;
{code}
*Line 195*:
{code:java}
NodeReportHandler nodeReportHandler =
new NodeReportHandler((SCMNodeManager) scmNodeManager);
{code}
The code is not wrong(got lucky on that one :) ), that is because the member
variable is not really needed once we pass this handler to eventQueue. But the
code can be confusing to read later, as the line 156 is not used at all.
Also if you revert the change to the Ctor of NodeReportHandler, you can avoid
the cast in line 195.
was (Author: anu):
Thanks for taking care of this issue. Some very minor comments on the patch.
* NodeReportHandler:java*:
* public NodeReportHandler(SCMNodeManager nm) ==> Spurious edit?
* Also you have lost the class comments for this class.
* Line 33: Why lose final? It might be a good idea to write a
Precondition.CheckNotNull for NodeManager.
* onMessage()*:
Precondition.CheckNotNull(nodeReport); since you are accessing members in
the call. It is obvious. But we have bugs it is easier to debug with asserts,
and please move both Preconditions to the start of the function.
*StorageContainerManager.java*:156:
You have added a new field, nodeReportHandler, however you seem to assign to a
local variable in the code.
*Line 156*:
{code}
/*
* Report Handlers
* */
private NodeReportHandler nodeReportHandler;
{code}
*Line 195*:
{code}
NodeReportHandler nodeReportHandler =
new NodeReportHandler((SCMNodeManager) scmNodeManager);
{code}
The code is not wrong(got lucky on that one :) ), that is because the member
variable is not really needed once we pass this handler to eventQueue. But the
code can be confusing to read later, as the line 156 is not used at all.
Also if you revert the change to the Ctor of NodeReportHandler, you can avoid
the cast in line 195.
> 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
>
>
> 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: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
