[GitHub] [hadoop] shvachko commented on a change in pull request #3201: Fgl saveloadfs

[GitBox <gi...@apache.org>]

shvachko commented on a change in pull request #3201:
URL: https://github.com/apache/hadoop/pull/3201#discussion_r674407665



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -1543,6 +1574,43 @@ public final void addRootDirToEncryptionZone(XAttrFeature xaf) {
     addEncryptionZone(rootDir, xaf);
   }
 
+  /**
+   * After the inodes are set properly (set the parent for each inode), we move
+   * them from INodeMapTemp to INodeMap.
+   */
+  public void moveInodes() throws IOException {

Review comment:
       Doesn't need to be public. Package private would do?
   Please also check other use of `public`. The most restrictive modifier should be used.

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -1860,6 +1928,21 @@ FSPermissionChecker getPermissionChecker()
     }
   }
 
+  public INode getInode(INode inode) {
+    return inodeMap.get(inode);
+  }
+  public INode getInodeFromTempINodeMap(long id) {
+    LOG.debug("getInodeFromTempINodeMap: id={}, TempINodeMap.size={}",
+        id, inodeMapTemp.size());
+    /*
+     * Convert a long inode id into an INode object. We only need to compare
+     * two inodes by inode id. So, it can be any type of INode object.
+     */
+    INode inode = new INodeDirectory(id, null,

Review comment:
       This will be a lot of new object instantiations during loading.
   Can we just define tempInodeMap : id -> INode
   to avoid creating new objects

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -1475,6 +1486,26 @@ void addYieldCount(long value) {
   public INodeMap getINodeMap() {
     return inodeMap;
   }
+  public GSet<INode, INodeWithAdditionalFields> getTempINodeMap() {

Review comment:
       Inconsistent naming. Getters should be named getMememberName().

##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -160,6 +164,8 @@ private static INodeDirectory createRoot(FSNamesystem namesystem) {
   private final int contentCountLimit; // max content summary counts per run
   private final long contentSleepMicroSec;
   private final INodeMap inodeMap; // Synchronized by dirLock
+  // Temp InodeMap used when loading an FS image.
+  private final GSet<INode, INodeWithAdditionalFields> inodeMapTemp;

Review comment:
       Can this be implemented so that `inodeMapTemp` is local variable, rather than a member of the class? It is used only during loading, and we don't need to keep it permanently after loading is done.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org