[ https://issues.apache.org/jira/browse/HDFS-517?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12745712#action_12745712 ]
Konstantin Shvachko commented on HDFS-517: ------------------------------------------ > Two general comments: As we talked about it, this logic belongs to changes related to block reporting and blockReceived logic. Not to this patch. > HdfsConstants.java 1. replaced with "created for replication and relocation only". 2. I disagree. BlockUCState is a block under construction state. Finalized block has other states like under-replicated, over-replicated, corrupt, etc. The states this enum defines referred to an under construction block only. Calling it a state would be too generic. I actually did this in the beginning, but realized later it is not correct. > BlockInfo.java 1. Constructors need to be protected, because we should always set the most strict restrictions possible. When a need arise we may relax them. There is no such need yet. 2. Yes NullPointerException will be a bug in our code. 3. I changed member blockState to blockUCState. You are right getters and setters should match the member name. See my comment about state above. 4. This is checked in convertLastBlockToUnderConstruction(). I really want to avoid double checking I'll add an assert here. Current append logic does not initialize targets in file inode. We will need to introduce it later. > BlockUnderConstruction.java 1. Added an assert. 2. Current code does not allow that. Lets loosen restrictions when needed rather than in advance. 3. Yes we should be prepared for repetitive calls if client gets disconnected. 4. No it starts from i = 1. 5. There is Block.set(id, len, gs). Set for state will be added when we will be changing uc states. > BlockManager.java 1. The JavaDoc explains it. 2. completeBlock() needs to know which block to complete. It does not make sense to search for it. > BlocksMap.java This is a correct observation. > FSdirectory.java No, because this is how addToParent is currently used. What makes sense is to merge addToParent and unprotectedAddFile(). They are the same. I leave it for after-append re-factoring. > FSNamesystem Changed error message. > INodeFile.java The advantage of returning a generic type is that you do not need to convert types if you know you need for example BlockInfoUnderConstruction. You just create a variable of that type and you are done. Otherwise you will need to perform an explicit type conversion. > Introduce BlockInfoUnderConstruction to reflect block replica states while > writing. > ----------------------------------------------------------------------------------- > > Key: HDFS-517 > URL: https://issues.apache.org/jira/browse/HDFS-517 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: name-node > Affects Versions: 0.21.0 > Reporter: Konstantin Shvachko > Assignee: Konstantin Shvachko > Fix For: Append Branch > > Attachments: Append-UCBlock-h265.patch, Append-UCBlock.patch, > Append-UCBlock.patch, Append-UCBlock.patch > > > Currently when a block is created its locations are stored in > {{INodeFileUnderConstruction.targets}}, which correspond to the last > allocated block. With the new append design we will need to keep track of > block replicas for several blocks rather than just the last one. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.