[
https://issues.apache.org/jira/browse/HDFS-4988?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13778471#comment-13778471
]
Junping Du commented on HDFS-4988:
----------------------------------
Thanks for the patch. Arpit!
I haven't finished my review, but just some initiative comments:
1. An important issue with following code is: we never get
InconsistentFSStateException now.
{code}
- String sid = getStorageID();
- if (!(sid.equals("") || ssid.equals("") || sid.equals(ssid))) {
+ String sid = sd.getStorageID();
+ if (!(sid == null || sid.equals("") ||
+ ssid.equals("") || sid.equals(ssid))) {
throw new InconsistentFSStateException(sd.getRoot(),
"has incompatible storage Id.");
}
-
- if (sid.equals("")) { // update id only if it was empty
- setStorageID(ssid);
+
+ if (sid == null) { // update id only if it was null
+ sd.setStorageID(ssid);
+ }
+
+ // Update the datanode UUID if present.
+ if (props.getProperty("datanodeUuid") != null) {
+ String dnUuid = props.getProperty("datanodeUuid");
+ setCachedDatanodeUuid(dnUuid);
+
+ if (getCachedDatanodeUuid() != null &&
+ getCachedDatanodeUuid().compareTo(dnUuid) != 0) {
+ throw new InconsistentFSStateException(sd.getRoot(),
+ "Root " + sd.getRoot() + ": DatanodeUuid=" + dnUuid +
+ ", does not match " + cachedDatanodeUuid + " from other" +
+ " StorageDirectory.");
+ }
{code}
I think previous way with checking mismatch of dnUuid and storageID (now is
cachedDatanodeUuid) is necessary. Do we want to fix something here?
2. Some minor comments below to see if you want to address:
a.
+ // For wire compatibility with older versions we transmit the StorageID
+ // which is the same as the DatanodeUuid. Since StorageID is a required
+ // field we pass the empty string if the DatanodeUuid is not yet known.c
"c" is not necessary.
b.
- if (storageInfo.numBlocks() == 0) {
+ if (storageInfo == null || storageInfo.numBlocks() == 0) {
storageInfo shouldn't be null here. Isn't it?
c.
+ public String getDatanodeUuid() {
+ if (id != null) {
+ return id.getDatanodeUuid();
+ } else {
+ return null;
+ }
+ }
We can be something simpler here "return id == null ? null :
id.getDatanodeUuid();"
d. I see we rename storage.getStorageId() to "storage.getCachedDatanodeUuid()".
I am fine with DatanodeUuid but do we need "cached" here? "cached" seems to
mean something temporary and can be easily fresh. Here it should be persistent
during DN life cycle. Shall we get rid of it?
> Datanode must support all the volumes as individual storages
> ------------------------------------------------------------
>
> Key: HDFS-4988
> URL: https://issues.apache.org/jira/browse/HDFS-4988
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode
> Reporter: Suresh Srinivas
> Assignee: Arpit Agarwal
> Attachments: HDFS-4988.01.patch, HDFS-4988.02.patch,
> HDFS-4988.05.patch, HDFS-4988.06.patch, HDFS-4988.07.patch
>
>
> Currently all the volumes on datanode is reported as a single storage. This
> change proposes reporting them as individual storage. This requires:
> # A unique storage ID for each storage
> #* This needs to be generated during formatting
> # There should be an option to allow existing disks to be reported as single
> storage unit for backward compatibility.
> # A functionality is also needed to split the existing all volumes as single
> storage unit to to individual storage units.
> # -Configuration must allow for each storage unit a storage type attribute.
> (Now HDFS-5000)-
> # Block reports must be sent on a per storage basis. In some cases (such
> memory tier) block reports may need to be sent more frequently. That means
> block reporting period must be on a per storage type basis.
> My proposal is for new clusters to configure volumes by default as separate
> storage unit. Lets discuss.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
