[
https://issues.apache.org/jira/browse/HDFS-7035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14189469#comment-14189469
]
Colin Patrick McCabe commented on HDFS-7035:
--------------------------------------------
{{Storage.java}}: remove unnecessary whitespace change
{code}
129 // Expose visibility for VolumeBuilder#commit().
130 public void addStorageDir(StorageDirectory sd) {
131 super.addStorageDir(sd);
132 }
{code}
This isn't needed because VolumeBuilder is in the same Java package as Storage,
so it can just call the parent method directly.
{code}
/** The unchanged locations that exist in the old configuration. */
{code}
Should be "existed in the old configuration"
{code}
builder.addBpStorageDirtectories(
{code}
Should be "directories" not "dirtectories"
{code}
167 // 2. Do transitions
168 // Each storage directory is treated individually.
169 // During startup some of them can upgrade or roll back
170 // while others could be up-to-date for the regular startup.
171 doTransition(datanode, sd, nsInfo, startOpt);
172 assert getCTime() == nsInfo.getCTime()
173 : "Data-node and name-node CTimes must be the same.";
{code}
This should be throwing an IOE, not an assert. Otherwise we're bringing down
the DataNode because someone tried to add a storage directory that wasn't
valid... not good.
{code}
327 // bpStorage does not add loaded volume immediately. The volume
will be
328 // added when calling builder.build() later. However, several
329 // members (e.g., Storage#layoutVersion, Storage#cTime will be
updated
330 // in BlockPoolSliceStorage#format() and
331 // BlockPoolSliceStorage#loadStorageDirectory. But since these
values are
332 // considered constant during the DataNode execution, we do not
revert the
333 // the changes on such members.
{code}
I think this comment belongs in the JavaDoc for the function. I also feel like
the current form of the comment is somewhat confusing. I would say something
like "prepareVolume creates a builder which can be used to add to the volume.
If the volume cannot be added, it is OK to discard the builder later."
removeVolumes: can you document in the JavaDoc for this function that even when
the IOE is thrown, the volumes are still removed?
+1 once these are addressed.
> Make adding volume an atomic operation.
> ---------------------------------------
>
> Key: HDFS-7035
> URL: https://issues.apache.org/jira/browse/HDFS-7035
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode
> Affects Versions: 2.5.0
> Reporter: Lei (Eddy) Xu
> Assignee: Lei (Eddy) Xu
> Attachments: HDFS-7035.000.combo.patch, HDFS-7035.000.patch,
> HDFS-7035.001.combo.patch, HDFS-7035.001.patch, HDFS-7035.002.patch,
> HDFS-7035.003.patch, HDFS-7035.003.patch, HDFS-7035.004.patch,
> HDFS-7035.005.patch, HDFS-7035.007.patch, HDFS-7035.008.patch,
> HDFS-7035.009.patch, HDFS-7035.010.patch, HDFS-7035.010.patch,
> HDFS-7035.011.patch, HDFS-7035.012.patch, HDFS-7035.013.patch
>
>
> It refactors {{DataStorage}} and {{BlockPoolSliceStorage}} to reduce the
> duplicate code and supports atomic adding volume operations. Also it
> parallels loading data volume operation: each thread loads one volume.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
