[
https://issues.apache.org/jira/browse/HDDS-187?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16527349#comment-16527349
]
Nanda kumar commented on HDDS-187:
----------------------------------
Thaks [~ajayydv] for working on this. Please find my review comments below.
*Possible NullPointerException*
{{StateContext}} has {{cmdStatusMap}} which holds the command to it's status
mapping.
1. Whenever we receive a command from SCM, {{HeartbeatEndpointTask}} adds an
entry to {{cmdStatusMap}} of {{StateContext}}.
2. {{CommandHandler}} updates the {{cmdStatusMap}} once the command is executed.
3. {{CommandStatusReportPublisher}} moves the entry from
{{StateContext#cmdStatusMap}} to {{StateContext#reports}} based on configured
interval.
4. {{HeartbeatEndpointTask}} picks the {{CommandStatusReportsProto}} from
{{StateContext#reports}} and sends it as part of next heartbeat.
In the above sequence step 2 and 3 are independent. If the command processing
takes time, step 3 will be executed before 2. In this case, we will get
NullPointerException in step 2.
*CommandHandlers*
All the {{CommandHandlers}} already have access to {{StateContext}}, which is
passed as part of {{handle}} call. We don't need it as part of constructor.
*HeartbeatEndpointTask.java*
No need to pass {{context}} to {{addCommandStatus}}, context is already an
instance variable.
*CloseContainerCommand.java*
We can make the new constructor private, as it should be invoked only through
{{getFromProtobuf}}.
*DeleteBlocksCommand.java*
new constructor can be made private
*CloseContainerCommandHandler.java*
Line:21 checkstyle - line length
*DeleteBlocksCommandHandler.java*
Line:21 unused import
Line:22 checkstyle - line length
*StateContext.java*
Line:23 checkstyle - line length
Incorrect java doc for {{getCommandStatusMap}}
*RegisterEndpointTask.java*
Spurious change.
*ReportPublisherFactory.java*
Line:22 checkstyle - line length
*SCMDatanodeHeartbeatDispatcher.java*
Line:21 & 52 checkstyle - line length
*ScmTestMock.java*
Line:21 checkstyle - line length
{code:java}
if(heartbeat.hasCommandStatusReport()){
heartbeat.getCommandStatusReport().getCmdStatusList().forEach( cmd -> {
cmdStatusList.add(cmd);
});
commandStatusReport.incrementAndGet();
}
{code}
can be refactored to
{code:java}
if(heartbeat.hasCommandStatusReport()){
cmdStatusList.addAll(heartbeat.getCommandStatusReport().getCmdStatusList());
commandStatusReport.incrementAndGet();
}
{code}
*TestEndPoint.java*
Line: 26 - 33 checkstyle - line length
*TestReportPublisher.java*
Line: 28 - 30 checkstyle - line length
*TestSCMDatanodeHeartbeatDispatcher.java*
Line: 26 & 34 checkstyle - line length
> Command status publisher for datanode
> -------------------------------------
>
> Key: HDDS-187
> URL: https://issues.apache.org/jira/browse/HDDS-187
> Project: Hadoop Distributed Data Store
> Issue Type: Bug
> Components: SCM
> Affects Versions: 0.2.1
> Reporter: Ajay Kumar
> Assignee: Ajay Kumar
> Priority: Major
> Fix For: 0.2.1
>
> Attachments: HDDS-187.00.patch, HDDS-187.01.patch
>
>
> Currently SCM sends set of commands for DataNode. DataNode executes them via
> CommandHandler. This jira intends to create a Command status publisher which
> will return status of these commands back to the 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]
