[
https://issues.apache.org/jira/browse/HDFS-11138?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15669116#comment-15669116
]
Anu Engineer commented on HDFS-11138:
-------------------------------------
Thank you for the patch, overall it looks very good. I have a bunch of very
minor comments. Since the patch is big, I might add more comments little later.
Please feel free to post a second patch if you like. Also for some reason we
did not get a Jenkins run, which would have given you all the CheckStyle and
other issues.
1. Could you please run checkstyle. I see a bunch of checkstyle warnings.
2. {{CBlockConfigKeys.java:21}} Comment about ozone, I think you meant cBlocks.
3. {{CBlockConfigKeys.java:39}}
//The port on CBlockManager node for jSCSI to ask
{noformat}
public static final int DFS_CBLOCK_JSCSI_PORT_DEFAULT = 50701;
public static final int DFS_CBLOCK_RPCSERVICE_PORT_DEFAULT = 50700;
{noformat}
I realize that you are trying to be consistent, however trunk has changed its
port map.
Namenode ports
----------------
50470 --> 9871
50070 --> 9870
8020 --> 9820
Secondary NN ports
---------------
50091 --> 9869
50090 --> 9868
Datanode ports
---------------
50020 --> 9867
50010 --> 9866
50475 --> 9865
50075 --> 9864
I know ozone still uses the old port map, we might have to go an fix it. You
might want to use a port that is free but closer to 98xx series.
4. {{public static final String DFS_CBLOCK_RPCSERVICE_IP_DEFAULT =
"127.0.0.1";}}
Did you want this to be 127.0.0.1 or 0.0.0.0 ? I would think you might want to
listen on 0.0.0.0
5.
{noformat}
58 public static final String DFS_CBLOCK_SERVICE_DELETE_FORCE_KEY =
59 "dfs.cblock.service.delete-force";
60 public static final boolean DFS_CBLOCK_SERVICE_DELETE_FORCE_DEFAULT =
61 false;
{noformat}
Slightly confused about how we use this, We have RPCs which take forceDelete as
a parameter, we also have forceDelete as a config key. I am trying to decide
which one takes precedence and why we need both.
6. More of a comment I not asking for a change here. We probably have 2 RPCs
from jSCSI server to cBlock server. getContainers and getLease. While I like
the fact that you have separated the interface that CLI depends upon and what
jscsi depends upon, I was wondering it is extra work. But now that it is done,
we should probably put that in.
7. {{ public void join()}} After we catch an exception, would you please add an
interrupt call ?
{noformat}
catch (InterruptedException e) {
Thread.currentThread().interrupt();
{noformat}
This is one of the dark corners of Java threading.
8. {{start, stop and join}} We have null check only in stop, I don't know if we
need it.
9. Nit: Log.info , you can use arguments instead of + operator.
10. {{ContainerDescriptor}} Can we please add an Index variable that tells us
what index this container is in the list of containers.
11. {{VolumeDescriptor.java}}
I am not able to understand why we have to maintain this map. Can you please
explain the use case ?
{{private HashMap<String, ContainerDescriptor> containerMap;}}
12. May I suggest that instead of converting this class to JSON and then
persisting it to local file -- Which we happen to do in the ozone test
implementation, I would suggest that we take the protobuf class as is -- and
the do toByteArray -- which will give you a byte stream which you can easily
persist to LevelDB. This works well for both keys and values.
13. We have a debugging line left out in the code: Line 204:
{{System.err.println("VolumeDesc...parse():" + jsonString);}}
14. {{CBlockServiceProtocol.java}}
{noformat}
33 void createVolume(String userName, String volumeName,
34 long volumeSize, int blockSize) throws IOException;
35
36 void createVolume(String userName, String volumeName,
37 long volumeSize) throws IOException;
{noformat}
Do we need both version of create ? if we set up the blockSize to have a
default size, then either the client can set the right value or you can rely on
default values in the protoc. Same comment about delete.
You can easily remove the second call by moving this to client side.
{noformat}
try {
51 if (request.hasBlockSize()) {
52 impl.createVolume(request.getUserName(),
request.getVolumeName(),
53 request.getVolumeSize(), request.getBlockSize());
54 } else{
55 impl.createVolume(request.getUserName(),
request.getVolumeName(),
56 request.getVolumeSize());
57 }
{noformat}
15. {{MountVolumeResponse}} Return a structure with index in
{{getContainerList}}. Please add some comments to this class, checkstyle is
going to complain about this. Also missing Licence header in this class.
16. {{CBlockClientServerProtocolServerSideTranslatorPB}}
{noformat}
62 for (int i=0;i<containers.size();i++) {
63 CBlockClientServerProtocolProtos.ContainerIDProto.Builder ID =
64
CBlockClientServerProtocolProtos.ContainerIDProto.newBuilder();
65 ID.setContainerID(containers.get(i));
66 ID.setIndex(i);
{noformat}
Shouldn't we just persiste the containerID Proto to the disk ?
17. You can eliminate the second {{get}} if you iterate with KeyEntry
{noformat}
for (String user : user2VolumeMap.keySet()) {
allVolumes.addAll(user2VolumeMap.get(user).values());
}
{noformat}
> 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
>
>
> 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]
