Details

FSVolume, is a part of FSDatasetInterface implementation, should not be referred outside FSDataset. A new FSVolumeInterface is defined. The BlockVolumeChoosingPolicy.chooseVolume(..) method signature is also updated.

Description

FSVolume is an inner class in FSDataset. It is actually a part of the implementation of FSDatasetInterface. It is better to define a new interface, namely FSVolumeInterface, to capture the abstraction.

FSVolume, is a part of FSDatasetInterface implementation, should not be referred outside FSDataset. A new FSVolumeInterface is defined. The BlockVolumeChoosingPolicy.chooseVolume(..) method signature is also updated.

Integrated in Hadoop-Mapreduce-trunk #984 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/984/)HDFS-2887. FSVolume, is a part of FSDatasetInterface implementation, should not be referred outside FSDataset. A new FSVolumeInterface is defined. The BlockVolumeChoosingPolicy.chooseVolume(..) method signature is also updated. (szetszwo)

Integrated in Hadoop-Hdfs-trunk #951 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/951/)HDFS-2887. FSVolume, is a part of FSDatasetInterface implementation, should not be referred outside FSDataset. A new FSVolumeInterface is defined. The BlockVolumeChoosingPolicy.chooseVolume(..) method signature is also updated. (szetszwo)

Integrated in Hadoop-Hdfs-trunk-Commit #1773 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1773/)HDFS-2887. FSVolume, is a part of FSDatasetInterface implementation, should not be referred outside FSDataset. A new FSVolumeInterface is defined. The BlockVolumeChoosingPolicy.chooseVolume(..) method signature is also updated. (szetszwo)

1. List<FSVolumeInterface> is needed for returning in FSDatasetInterface.getVolumes(). We cannot use List<FSVolume> for returning since List<FSVolume> cannot be cast to List<FSVolumeInterface>. We may create a new list in getVolumes() or have two lists (a List<FSVolume> and a List<FSVolumeInterface> as I did in h2887_20120203.patch) in FSVolumeSets but both solutions are inefficient. They create unnecessary objects. So, I use List<FSVolumeInterface> in FSVolumeSets and cast FSVolumeInterface objects to FSVolume inside FSDataset.

Tsz Wo Nicholas Sze
added a comment - 08/Feb/12 20:33 Suresh, thanks for the review.
1. List<FSVolumeInterface> is needed for returning in FSDatasetInterface.getVolumes(). We cannot use List<FSVolume> for returning since List<FSVolume> cannot be cast to List<FSVolumeInterface>. We may create a new list in getVolumes() or have two lists (a List<FSVolume> and a List<FSVolumeInterface> as I did in h2887_20120203.patch) in FSVolumeSets but both solutions are inefficient. They create unnecessary objects. So, I use List<FSVolumeInterface> in FSVolumeSets and cast FSVolumeInterface objects to FSVolume inside FSDataset.
2. This is a good idea. Let's do it in a separated JIRA.
3. Definitely. I already have marked this as an incompatible change.

Currently there are several places in FSVolumeSet where FSVolumeInterface is cast to FSVolume. FSVolumeSet should continue to take FSVolume in the constructor. That way outside FSDataset the only thing exposed is FSVolumeInterface. Inside FSDataset, we could continue to use FSVolume?

In a separate Jira perhaps we should add to FSDatasetInterface, isDataScanSupported() and isDirectoryScanSupported. Perhaps we could move scanners starting to FSDataset altogether and add a method, startScanners()?

When committing this change, this should be marked incompatible, due to BlockVolumeChoosingPolicy change

Suresh Srinivas
added a comment - 08/Feb/12 20:21 Change make the code much cleaner. Comments:
Currently there are several places in FSVolumeSet where FSVolumeInterface is cast to FSVolume. FSVolumeSet should continue to take FSVolume in the constructor. That way outside FSDataset the only thing exposed is FSVolumeInterface. Inside FSDataset, we could continue to use FSVolume?
In a separate Jira perhaps we should add to FSDatasetInterface, isDataScanSupported() and isDirectoryScanSupported. Perhaps we could move scanners starting to FSDataset altogether and add a method, startScanners()?
When committing this change, this should be marked incompatible, due to BlockVolumeChoosingPolicy change

Hadoop QA
added a comment - 08/Feb/12 03:53 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12513748/h2887_20120207.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 21 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1854//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1854//console
This message is automatically generated.

Hadoop QA
added a comment - 03/Feb/12 13:38 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12513119/h2887_20120203.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 18 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1837//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1837//console
This message is automatically generated.

BlockVolumeChoosingPolicy declares the chooseVolume(..) method with FSVolume but FSVolume is a private API. The patch is going to change it to use FSVolumeInterface. In this sense, this is an incompatible change.

Tsz Wo Nicholas Sze
added a comment - 03/Feb/12 11:21 BlockVolumeChoosingPolicy declares the chooseVolume(..) method with FSVolume but FSVolume is a private API. The patch is going to change it to use FSVolumeInterface. In this sense, this is an incompatible change.

FSVolume is an inner class in FSDataset. It is actually a part of the implementation of FSDatasetInterface. It is better to define a new interface, namely FSVolumeInterface, to capture the abstraction.