[ 
https://issues.apache.org/jira/browse/HDFS-12159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16123912#comment-16123912
 ] 

Xiaoyu Yao commented on HDFS-12159:
-----------------------------------

Thanks [~anu] for the update. The latest patch looks good to me. Below are some 
new comments.

Ozone.proto
Line 78: Do we need a separate STAND_ALONE? 
This will be either RATIS of size 1 or CHAINED of size 1, which will be more 
generic.

StorageContainerLocationProtocol.proto
Line 121: should we name it nodepool to be consistent?

StorageContainerLocationProtocol.java
Line 38-39: We are changing to use protobuf types like 
OzoneProtos.ReplicationFactor/ReplicationType
directly in APIs. Is this the style want to do moving forward in other places 
to avoid the conversion 
between PB types and the Ozone types?

ContainerOperationClient.java
NIT: line 108-109 java doc needs update
(or we should use @inherit for the @override methods to avoid double document 
overhead)

ScmClient.java
NIT: line 102 java doc incomplete

StorageContainerLocationProtocolClientSideTranslatorPB.java
Line 219: NIT: should we call allocatePipeline instead of 
createReplicationPipeline to be consistent?
In the container case, the caller of the allocateContainer could be different 
from
the client actually create containers on datanodes. 

StorageContainerLocationProtocol.proto
Line 166: based on the comments here, we seems to let SCM talks to datanodes to 
create the pipeline,
which is different from how container is allocated/created. Is this correct? If 
that is the case, pipeline
will not need to separate states between allocate/create.

DatanodeStateMachine.java
Line 91-94: these can be removed.

StateContext.java
Line 102: NIT: can we define something like INVALID_PORT = -1 somewhere like 
OzoneConsts.java?

XceiverServerRatis.java
Line 98-100: can be removed or change output to debug log
Line 111: should we have a special configuration keys for remapping of the 
storageDir for Ratis in
MinOzoneCluster mode?

OzoneContainer.java
Line 196: use OzoneConsts#INVALID_PORT?

ContainerMapping.java
Line 165: NIT: unnecessary line break

PipelineManager.java
Line 41: do we assume the Pipleline returned in OPERATIONAL state?
Or do we allow getPipleLine by state for pipeline cleanup?
Line 43: NIT: can we rename getReplicationPipeline to getPipeline?
Line 63: NIT: can we call it getMembers? That matches well with the javadoc 
above it. 
Line 68: NIT: rename to updatePipeline()? we may also need updatePipeline with
states from PipelineManager.


StandaloneManagerImpl.java
Line 59: do we need more than one datanodes on standalong replication? 
I would suggest we make this a ChainedManagerImpl.java and support standalone 
as a special case of it and leave the the multi-node chained replication as 
unimplemented.
This way, we do not need to maintain the standaloneManager. We could do that 
in a separate patch since this one is already large.

> Ozone: SCM: Add create replication pipeline RPC
> -----------------------------------------------
>
>                 Key: HDFS-12159
>                 URL: https://issues.apache.org/jira/browse/HDFS-12159
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>    Affects Versions: HDFS-7240
>            Reporter: Anu Engineer
>            Assignee: Anu Engineer
>             Fix For: HDFS-7240
>
>         Attachments: HDFS-12159-HDFS-7240.001.patch, 
> HDFS-12159-HDFS-7240.002.patch, HDFS-12159-HDFS-7240.003.patch
>
>
> Add an API that allows users to create replication pipelines using SCM.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org

Reply via email to