HDFS-15961. standby namenode failed to start ordered snapshot deletion is enabled while having snapshottable directories #2881
Conversation
…n is enabled while having snapshottable directories.
There was a problem hiding this comment.
Thanx @bshashikant for the fix, Can you extend a test covering the fix as well
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Added the test case. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanx @bshashikant for the fix, this fix seems correct. |
|
@smengcl , can you please have a look again? |
|
I think We should hold this off, I think the code has issues, as I said. Earlier I thought, there is some catch but I don’t think. It is misbehaving only. Ideally such features should go in a branch first.... |
@ayushtkn , IMO its not a misbehaviour here. Once the snapshotTrash feature is enabled, the .Trash directory has to be present to make the feature work. As you can have pre existing snapshottable directories , one way to create the .Trash inside these was to create right after the startup is done. The other solution was to explicitly provision the .Trash using a cmd line option. The choice was made to do it automatically on restart, and fail the Namenode in case any any issues occur. The other solution is not fail the namenode , but log an warning and later, if any Trash operations gets performed inside the snapshottable root, will fail. The discussion is here: #2682 (comment) |
Coming to think of it, if providing an external command to create the Trash directory by admins is feasible and makes sense, i think its ok to remove the NN startup logic to create Trash directories inside snapshottable root. |
…g failure on NN startup.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanx @bshashikant I just had a cursory look, Won't be able to spend time for a thorough review anytime soon. You can proceed, if others are convinced.
I think @arp7 has also validated HDFS-15614 and was convinced with the changes, so things should be good only, I might have messed up.
Arpit If you can spare some time, do give a check to this one too, just changes from EXIT to LOG in case of failure got changed extra.
| @@ -98,6 +98,7 @@ public void setupCluster() throws Exception { | |||
| conf.setInt(DFSConfigKeys.DFS_BLOCK_SIZE_KEY, BLOCK_SIZE); | |||
| conf.setInt(DFSConfigKeys.DFS_HEARTBEAT_INTERVAL_KEY, 1); | |||
| conf.setInt(DFSConfigKeys.DFS_HA_TAILEDITS_PERIOD_KEY, 1); | |||
| conf.setBoolean("dfs.namenode.snapshot.trashroot.enabled", false); | |||
There was a problem hiding this comment.
Can we move this config in DFSConfigKyes and use it from there, As of now I think it is defined in FsNamesystem which doesn't makes, The value of this I guess is exposed via getServerDefaults also so no point keeping it in FsN
There was a problem hiding this comment.
It is intended to be an hidden config similar to ordered snapshot deletion config. I would prefer to keep it as it is now.
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
|
+1 on the latest change. Let's merge the fix to trunk before I have filed HDFS-15997 to implement |
|
The failures are nit related to the patch. |
…n is enabled while having snapshottable directories (apache#2881)
…n is enabled while having snapshottable directories (apache#2881) Conflicts: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestHASafeMode.java Ref: CDPD-24815 Change-Id: Ibc06963223db50b96d4cbdf64817410db9c8a365
Please refer https://issues.apache.org/jira/browse/HDFS-15961.