-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-16128: Added support for saving/loading an FS Image for fine-grain locking #3201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeMap.java
Outdated
Show resolved
Hide resolved
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/INodeMap.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
prasad-acit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for commening late, i have added 2 more comments but forgot to submit them. Plz take a look.
| LOG.debug("getInodeFromTempINodeMap: id={}, TempINodeMap.size={}", | ||
| id, inodeMapTemp.size()); | ||
| INode inode = new INodeWithAdditionalFields(id, null, | ||
| new PermissionStatus("", "", new FsPermission((short) 0)), 0, 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PermissionStatus - a static object can be used here instead of creating new one for every call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we define a PermissionStatus object as a static object, then it looks like we have to define that static object inside the class, instead of this function. Would that create some unnecessary confusion, given we need that object only for this function?
Happy to make it a static object if we can define it inside this function.
...fs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
Outdated
Show resolved
Hide resolved
shvachko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @xinglin could you take care of some checkstyle and spotbug warnings from the Jenkins build.
Would be good to have a unit test, which saves, loads, and reloads the image. Unless the one already exists.
...fs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
Outdated
Show resolved
Hide resolved
...fs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
Outdated
Show resolved
Hide resolved
...fs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
Outdated
Show resolved
Hide resolved
...fs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
+1 |
…deMap.get(long inode).
…odeWithAdditionalFields> for tempINodeMap.
xinglin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Konstantin,
I should have addressed your four specific comments. The only comment which has not been addressed is to add unit tests. Will work on that later on.
…rtitionedGSet. Contributed by Xing Lin. (#3201)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…rtitionedGSet. Contributed by Xing Lin. (#3201)
|
I re-committed PR to reflect latest changes. |
|
close it. Appreciated all the comments I received for this commit and Thanks @shvachko for committing it! |
…rtitionedGSet. Contributed by Xing Lin. (#3201)
Test:
We create 1 million dirs and then stop and re-start the namenode. Namenode restarts successfully.
Log output from the namenode.