Block management should only care about blocks but not the namespace. It should not know anything about the namespace. It makes sense to define a BlockCollection interface for INodeFile and INodeFileUnderConstruction. We probably need two interfaces, one for mutable collections and another one for immutable collections.

Tsz Wo Nicholas Sze
added a comment - 03/May/12 18:54 Block management should only care about blocks but not the namespace. It should not know anything about the namespace. It makes sense to define a BlockCollection interface for INodeFile and INodeFileUnderConstruction. We probably need two interfaces, one for mutable collections and another one for immutable collections.

Daryn Sharp
added a comment - 03/May/12 21:05 This is probably just a step in the right direction, but it seems odd that BlocksMap#getINode isn't renamed to getBlockCollection since it now returns a BlockCollection .
My only concern is why pathname handling is moved into INode ? Inodes should be "dumb" model objects, with a controller that handles path manipulation – ie. FSDirectory .

Tsz Wo Nicholas Sze
added a comment - 03/May/12 22:22
BlockCollection and MutableBlockCollection should be in the blockmanagement package. Then, services other than namenode can use them. Also, the javadoc should not use the term "file".
Remove BlockCollection.isDirectory() and the assert statement in BlockManager.getReplication(Block block).
Remove BlockCollection.isUnderConstruction() and replace the use of it in blockmanagement by checking instanceof MutableBlockCollection.
Rename getFullPathName() to getName(). It is the name of the collection but not necessarily a path name.
I agree with Daryn that we should rename BlocksMap.getINode(..) and, in addition, the local variable names such as fileInode. We could do the rename separately.

Tsz Wo Nicholas Sze
added a comment - 03/May/12 22:49 After the patch, we could easily change INodeFile and INodeFileUnderConstruction to package private. We only have to update a few unit tests. Nice work!

John George
added a comment - 04/May/12 15:07 Thanks for taking a look Daryn and Nicholas. Attaching a new patch that addresses your comments.
Moved getFullPathName(INode[], int) back to FSDirectory.
Filed HDFS-3369 to change 'inode' names more appropriate to BlockCollection
Addressed Nicholas's comments.

Move BlockCollection.setBlock(..) to MutableBlockCollection and change the parameter in BlockManger.forceCompleteBlock(..) and completeBlock(..) to MutableBlockCollection. We should also move setBlock(..) from INodeFile to INodeFileUnderConstruction (this can be done separately.)

Tsz Wo Nicholas Sze
added a comment - 04/May/12 22:05
Move BlockCollection.setBlock(..) to MutableBlockCollection and change the parameter in BlockManger.forceCompleteBlock(..) and completeBlock(..) to MutableBlockCollection. We should also move setBlock(..) from INodeFile to INodeFileUnderConstruction (this can be done separately.)
need to update the following
//FSNamesystem.listCorruptFileBlocks(..)
String src = FSDirectory.getFullPathName(inode);
revert the following: we don't need to cast it for null checking
//FSNamesystem
private boolean isValidBlock(Block b) {
- return (blockManager.getINode(b) != null );
+ return ((INodeFile) blockManager.getINode(b) != null );
}
Need to rewrite the javadoc in BlockCollection and MutableBlockCollection
do not use "file"
do not mention INodeFile and INodeFileUnderConstruction
revert the changes in TestBlockManager and FSEditLogLoader.

Move BlockCollection.setBlock(..) to MutableBlockCollection and change the parameter in BlockManger.forceCompleteBlock(..) and completeBlock(..) to MutableBlockCollection. We should also move setBlock(..) from INodeFile to INodeFileUnderConstruction (this can be done separately.)

Need to rewrite the javadoc in BlockCollection and MutableBlockCollection
do not use "file"
do not mention INodeFile and INodeFileUnderConstruction
revert the changes in TestBlockManager and FSEditLogLoader.

John George
added a comment - 07/May/12 19:18 Move BlockCollection.setBlock(..) to MutableBlockCollection and change the parameter in BlockManger.forceCompleteBlock(..) and completeBlock(..) to MutableBlockCollection. We should also move setBlock(..) from INodeFile to INodeFileUnderConstruction (this can be done separately.)
Filed HDFS-3379
need to update the following
//FSNamesystem.listCorruptFileBlocks(..)
String src = FSDirectory.getFullPathName(inode);
Reverted the getFullPathName change since it was not really necessary to complete this JIRA.
revert the following: we don't need to cast it for null checking
//FSNamesystem
private boolean isValidBlock(Block b)
Unknown macro: {
- return (blockManager.getINode(b) != null);
+ return ((INodeFile) blockManager.getINode(b) != null);
}
Need to rewrite the javadoc in BlockCollection and MutableBlockCollection
do not use "file"
do not mention INodeFile and INodeFileUnderConstruction
revert the changes in TestBlockManager and FSEditLogLoader.
done

Hadoop QA
added a comment - 08/May/12 00:18 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12525926/HDFS-3363.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+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 hadoop-hdfs-project/hadoop-hdfs.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2386//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2386//console
This message is automatically generated.