[jira] [Comment Edited] (HDFS-13116) Ozone: Refactor Pipeline to have transport and container specific information

["Anu Engineer (JIRA)" <ji...@apache.org>]

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

Anu Engineer edited comment on HDFS-13116 at 2/7/18 9:04 PM:
-------------------------------------------------------------

[~msingh] Thanks for working on this patch. It looks very good. I have some very minor comments.
 * Conduit --> Something like PipelineInfo
 * interface to Abstract --I am wondering if this makes future changes less flexible. For example, if we have erasure coded pipelines, I am not sure if this abstract class implementation would work. But for the current case of stand-alone and Ratis, I do see it works well.

 * {{PipelineManager#getPipeline}}
{code:java}
  if (conduit == null) {
      LOG.error("Get conduit call failed. We are not able to find free nodes" +
          " or operational conduit.");
    }
    return new Pipeline(containerName, conduit);
 {code}
Shouldn't we throw after the error, or do you want to return a new pipeline with conduit equal to null?

{{PipelineManager#findOpenConduit}}
Should this function be protected via a synchronized keyword or a lock?

Here is what I am thinking – please correct me if I am wrong.

– This is the point of check
{code:java}
  if (activeConduits.size() == 0) {
{code}
– This is the point of use.
{code:java}
	private int getNextIndex() {
    return conduitsIndex.incrementAndGet() % activeConduits.size();
    }
{code}
What happens if the activeConduits size becomes zero? is that possible?


was (Author: anu):
[~msingh] Thanks for working on this patch. It looks very good. I have some very minor comments.
* Conduit --> Something like PipelineInfo
* interface to Abstract --I am wondering if this makes future changes less flexible. For example, if we have erasure coded pipelines, I am not sure if this abstract class implementation would work. But for the current case of stand-alone and Ratis, I do see it works well.

* {{PipelineManager#getPipeline}}
{code}
  if (conduit == null) {
      LOG.error("Get conduit call failed. We are not able to find free nodes" +
          " or operational conduit.");
    }
    return new Pipeline(containerName, conduit);
 {code}
 Shouldn't we throw after the error, or do you want to return a new pipeline with conduit equal to null?

{{PipelineManager#findOpenConduit}}
Should this function be protected via a synchronized keyword or a lock? 

Here is what I am thinking -- please correct me if I am wrong.

-- This is the point of check 
{code}
  if (activeConduits.size() == 0) {
{code}

--  This is the point of use.
{code}
	private int getNextIndex() {
    return conduitsIndex.incrementAndGet() % activeConduits.size();
    }
{code}
What happens if the activeConduits size becomes zero? ispossible?ible ?







> Ozone: Refactor Pipeline to have transport and container specific information
> -----------------------------------------------------------------------------
>
>                 Key: HDFS-13116
>                 URL: https://issues.apache.org/jira/browse/HDFS-13116
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>    Affects Versions: HDFS-7240
>            Reporter: Mukul Kumar Singh
>            Assignee: Mukul Kumar Singh
>            Priority: Major
>             Fix For: HDFS-7240
>
>         Attachments: HDFS-13116-HDFS-7240.001.patch, HDFS-13116-HDFS-7240.002.patch, HDFS-13116-HDFS-7240.003.patch, HDFS-13116-HDFS-7240.004.patch, HDFS-13116-HDFS-7240.005.patch, HDFS-13116-HDFS-7240.006.patch
>
>
> Currently pipeline has information about both the container as well Transport layer. This results in cases where a new pipeline (i.e. transport) information is allocated for each container creation.
> This code can be refactored so that the Transport information is separated from the container, then the {{Transport}} can be shared between multiple pipelines/containers.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

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