hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anu Engineer (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-11138) Block Storage : add block storage server
Date Thu, 17 Nov 2016 23:59:01 GMT

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

Anu Engineer commented on HDFS-11138:
-------------------------------------

[~vagarychen] Thanks for updating the patch. Overall it is looking quite good. Some very minor
comments and questions.

1. Can you please clarify these comments. It is hard to know the difference between local
client and cblock client.
{noformat}
CBlockServiceProtocolPB - The protocol that local client tool uses to talk to CBlock server.
CBlockClientServerProtocolPB - Protocol that cblock client uses to talk to cblock server.
{noformat}

2. Don't understand this logging message here. if user does not specify a block size we will
use this value. But why log that in startup ? 
{noformat}
  LOG.info("CBlock manager initialized, with default block size: {}",DFS_CBLOCK_SERVICE_BLOCK_SIZE_DEFAULT);
{noformat}

3. In {{VolumeDescriptor.java}} shouldn't this {{private List<String> containerIdOrdered;}}
be {{private List<ContainerDescriptor> containerIdOrdered;}}

4. Also a related question, does it make sense to make {{addContainer}} automatically call
{{setContainerIDs}}. That way the user of this API does not need to know about keeping these
two lists. In {{StorageManager.java}} that would allow you to eliminate maintaining an extra
list.

5.   public static VolumeDescriptor parse(String jsonString)  -- do we need this ?

6. in {{CBlockClientServerProtocolServerSideTranslatorPB#mountVolume}}
{noformat}
 List<String> containers = result.getContainerList();
        for (int i=0; i<containers.size(); i++) {
          CBlockClientServerProtocolProtos.ContainerIDProto.Builder id =
              CBlockClientServerProtocolProtos.ContainerIDProto.newBuilder();
          id.setContainerID(containers.get(i));
          id.setIndex(i);
          resp.addAllContainerIDs(id.build());
        }
 {noformat}
is it possible to return a ContainerDescriptor instead of a List of strings in the result,
that way we can avoid this for loop for numbering.

7. in {{StorageManager#addVolume}}
LOGGER.error -- we should log the error.  it can be rewritten as
{{LOGGER.error("Getting container failed! Container:{} error:{}", containerId, e);}}

if you like you can use this parameter passing style for all the log messages instead of string
addition.

8. Same comment in addVolume, we should log the error.

9.  {{StorageManager#deleteVolume}}  
{noformat}
storageClient.deleteContainer(containerID);
      } catch (IOException e) {
        LOGGER.error("Error deleting container:" + containerID);
      }
{noformat}
Looks like we ignore the exception of deleting a specific container and continue the delete
operation. We do log it, but does the caller get any error back ? Shouldn't we propagate this
 error back ? Let us also log the exception in the log.



> Block Storage : add block storage server
> ----------------------------------------
>
>                 Key: HDFS-11138
>                 URL: https://issues.apache.org/jira/browse/HDFS-11138
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: hdfs
>            Reporter: Chen Liang
>            Assignee: Chen Liang
>         Attachments: HDFS-11138-HDFS-7240.001.patch, HDFS-11138-HDFS-7240.002.patch,
HDFS-11138-HDFS-7240.003.patch, HDFS-11138-HDFS-7240.004.patch
>
>
> This JIRA adds the skeleton for server side code, which is one of the core components
in block storage. For performance concerns, the server does not handle any actual read/write
operation but serving primarily as a meta data server. It provides four APIs:
> # create volume : which will call into underlying container layer to allocate containers
> # delete volume : delete a specific volume (as well as its containers)
> # info volume : return information of a specific volume
> # list volume : list all volumes
> Note that this is still subject to potentially major changes. Features such as persistence
are missing.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

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


Mime
View raw message