[
https://issues.apache.org/jira/browse/HDDS-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16644932#comment-16644932
]
Nanda kumar commented on HDDS-587:
----------------------------------
[~ljain], thanks for reporting and working on the patch, overall it looks good
to me. Some minor comments,
*PipelineManager:*
We don't need {{TODO: APIs - getReplicationPipeline}}.
Rename suggestion: {{addContainer}} to {{addContainerToPipeline}}
Rename suggestion: {{removeContainer}} to {{removeContainerFromPipeline}}
We also need an API to get the list of containers, given a pipeline id.
Something like {{getContainersInPipeline}}
*SCMPipelineManager:*
L83: It should be {{SCM_PIPELINE_DB}} not {{SCM_CONTAINER_DB}}.
We can remove the lock from {{initializePipelineState}} as it is called only
from the constructor.
{{createContainerPlacementPolicy}} should be moved into pipeline selector, in
future, it should be possible for us to use two different logic/placement
policy for different types of pipeline. For this, we might need to pass on the
Configuration object to PipelineProvides via PipelineFactory.
In {{finalizePipeline}}, add a TODO that we have to fire events to close all
the containers which are part of this pipeline.
The TODO in {{close}} can be removed, no one should use PipelineManager after
the close is called. If a check is definitely required, we can add it later.
*PipelineStateManager:*
Rename suggestion: {{addContainer}} to {{addContainerToPipeline}}
Rename suggestion: {{removeContainer}} to {{removeContainerFromPipeline}}
*PipelineStateMap:*
L:42 You can remove the TODO and also the lock, yes we don't need a lock in
PipelineStateMap as we have one in SCMPipelineManager.
Rename suggestion: {{addContainer}} to {{addContainerToPipeline}}
Rename suggestion: {{removeContainer}} to {{removeContainerFromPipeline}}
In {{getPipeline}}, we can always throw an exception if the pipeline is not
found. The caller has to handle the exception. One improvement that we can do
is to add a new Exception called PipelineNotFoundException, this can be done in
a follow-up jira.
The above comment is also applicable for
{{getContainer}}/{{getContainersInPipeline}} call.
{{throws IOException}} can be removed from method signature of
{{isClosingOrClosed}} and {{isOpen}}
*Pipeline:*
Change {{Set<DatanodeDetails> nodes}} to {{List<DatanodeDetails> nodes}}. When
we implement rack-awareness, the order of nodes in the pipeline will become
important. We have to maintain the order of datanodes in {{Pipeline}}
We should have proper {{hashCode}} implementation. You can use
{{EqualsBuilder}} and {{HashCodeBuilder}} from apache-commons-lang to implement
{{equals}} and {{hashCode}} methods.
Checkstyle issues in {{Builder}} class.
*PipelineID:*
remove {{public static PipelineID valueOf(String id)}} method. It feels like
this method can take any arbitrary string to construct PipelineID, which is not
true.
> Add new classes for pipeline management
> ---------------------------------------
>
> Key: HDDS-587
> URL: https://issues.apache.org/jira/browse/HDDS-587
> Project: Hadoop Distributed Data Store
> Issue Type: Bug
> Components: SCM
> Reporter: Lokesh Jain
> Assignee: Lokesh Jain
> Priority: Major
> Attachments: HDDS-587.001.patch, HDDS-587.002.patch
>
>
> This Jira adds new classes and corresponding unit tests for pipeline
> management in SCM. The old classes will be removed in a subsequent jira.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
