[
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: [email protected]
For additional commands, e-mail: [email protected]
