I will send comments on the design doc later.
Comments (lot of them are nits...):
1. Design uses BackupNode as a subclass of NameNode. In future where Backup can transition to Active or Active can transition to Backup, this relationship seems stringent. Capturing active, backup as the state of Namenode might be a better idea.
2. If EditLogBackupOutputStream fails to write to the backup, how does active and backup recover from this condition. Looks like the current mechanism to recover a failed stream to disk is not workable for stream to backup.
1. General comments:
1.1. For consistently use NameNode and DataNode intead of name-node and data-node
1.2. Many class contructors call default constructor of the super class super(). This is unnecessary.
1.3. Many if statements are not enclosed in curly brackets. Do we follow this java coding convention?
2.1. is shown as changed file. It should be new add and UnregisteredDatanodeException.java should be deleted.
2.2. Class comment still only talks about Datanode. It should be made generic to all types of node
2.3. Constructor can be change to take
3.1. Class comment seems to indicate this is NameNode specific. Comment should indicate that it is generic.
4.1. Additional description for the class to indicate that this is sent in response to startCheckpoint NamenodeProtocol command would be good.
5.1. In class comments why are the changes made for previous protocol version removed (in this case version 2). Isn't retaining this history a good idea for better documentation?
5.2. getBlocks() comment suggests that it i used by Balancer. Why have this comment if this could be used by things other than Balancer in the future.
5.3. Description for methods versionRequest, errorReport, endCheckpoint,
5.4. startCheckpoint() good to indicate direction of the message (from backup to active)
5.5. Move static variables for Journal pipeline action to the beginning of the class
5.6. journal() better documentation on what it does and the direction of this message.
6.1. Should the action static ints be moved to NamenodeProtocol.java? This is similar to Journal actions defined in NamenodeProtocol.java
7.1. It is better to be explicit about printing NamenodeRole as "Backup NameNode" and "Checkpoint NameNode" instead of just "Backup Node" and "Checkpoint Node".
8.1. In class comments </li> and </ol> are missing.
8.2. No need to defined LOG as it is inherited from NameNode
8.3. Using dfs.backup.address and dfs.backup.http.address means backup is defined at the install time. To change backup to active namenode, configuration change needs to be made. This could be restrictive in the future when a backup node can automatically transtion to become active.
8.4. When starting nodes, we may need to check the address read from configuration with the local node address for http.address etc.
8.5. Comment "NameNodeCommon methods" should change to "Common NameNode methods"
8.6. import org.apa/che.commons.logging.*; is not used
8.7. Replace import java.io.; import java.net. to import specific ones IOException, InetSocketAddress, SocketTimeoutException
8.8. handleShake() use NamenodeProtocol.NOTIFY instead of DatanodeProtocol.NOTIFY in errorReport
8.9. Should BackupNode override Namenode.errorReport(), in order to not do the processing of releasing backup node?
8.10. Should BackupNode throw exception for NamenodeProtocol methods NameNode.journalSize()?
8.11. stop() - is there a need for RPC.stopProxy(namenode). Does super.stop() take care of it?
9.1. doCheckPoint() calls backupNode.setCheckpointState(CheckpointStates.ROLLED_EDITS). Should we do this in startJournalSpool(). That way for some reason startCheckpoint() to active fails, the checkpointer state remains consistent. doCheckPoint() should wait for state to reach CheckpointStates.ROLLED_EDITS to make sure the active has rolled edits before proceeding further with checkpoint. This helps in any boundary conditions where checkpoint could miss edits that might still come from active to previous edits log.
9.2. doCheckpoint() should call convergeJournalSpool() for BackupNode of type BACKUP only
9.3. Given that Checkpointer is looking into internals of BackupNode so much, should it be made private inner class of BackupNode?
9.4. Should downloadCheckpoint() and uploadCheckpoint() have registrationID or some other token that is ephemeral perhaps for validating the access. Otherwise these URLs could be misused to mess with the Namenode state. This could be done in another jira.
9.5. run() Use either period or checkpointPeriod in the while loop. Also we can keep 1000 * period calculated outside the while loop.
10.1. For better readability NameNode could support instead of node.getRole() == NamenodeRole.BACKUP, method isRole(NamenodeRole.BACKUP) or isActive(), isBackup(), isCheckpointer().
10.2. Not sure what the comment // trash in NameNode constructor is far...
11.1. Need to create a separate jira to enhance this to include additional information that could help in catching error conditions such as backup node FSImage number of files not matching the active node etc. Currently any condition where backup does not match the active goes undetected.
12.1. Should constructor be using parameter name to set the member address?
12.2. getType() should return JournalType.BACKUP
12.3. Class comments need to be updated
13.1. getStorageDirs() FSNamesystem.LOG can just be LOG()
14.1. minor nit - startCheckpoint() comment has name-name.
14.2. startCheckpoint() should some of the if conditions be else if where msg is set
14.3. Adding parentheses to the if condition that combines || with && will make it more readable