Xiaoyu Yao commented on HDFS-12159:

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

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 

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

Line 38-39: We are changing to use protobuf types like 
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?

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

NIT: line 102 java doc incomplete

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 
the client actually create containers on datanodes. 

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.

Line 91-94: these can be removed.

Line 102: NIT: can we define something like INVALID_PORT = -1 somewhere like 

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?

Line 196: use OzoneConsts#INVALID_PORT?

Line 165: NIT: unnecessary line break

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.

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 
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

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