Simple patch implements the above. Does not add unit tests since it's a straight refactor of existing code, and that code is covered by many existing tests.

+1 lgtm.

thanks Bikas. Quick question for reviewers: when I moved this code, I noticed the canRead() check. Currently if the file exists but can't be read, it returns the default value. I thought this was a little suspicious. Anyone adverse to removing that check, so that we throw an exception if it exists but we can't read it? Or better to keep this as a straight refactor and file a follow-up to think about that?

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12521220/hdfs-3190.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in .

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2173//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2173//console

This message is automatically generated.

Re-attaching to get a fresh QA run, since it's been a couple months.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12531319/hdfs-3190.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2616//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2616//console

This message is automatically generated.

It's good to see some cleanup.

Hmm. LongContainingFile just seems like a containing file that is long.

How does SingleValueFile seem as a class name? Then you could have SingleValueFile#readLong and SingleValueFile#writeLong.

+  public static long read(File file, long defaultVal) throws IOException {
+    long txid = defaultVal;
+    // TODO: canRead below seems wrong
+    if (file.exists() && file.canRead()) {
+      BufferedReader br = new BufferedReader(new FileReader(file));
+      try {
+        txid = Long.valueOf(br.readLine());
+        br.close();
+        br = null;
+      } finally {
+        IOUtils.cleanup(LOG, br);
+      }
+    }
+    return txid;
+  }

Yeah, I agree that the canRead() seems unecessary.
It's unfortunate that the BufferedReader constructor always throws a FileNotFound exception, even if the file is found but simply is not readable. However, despite the misnamed exception, if you get rid of the canRead(), you should have the semantic that file not found => default, file not readable => IOException, which I think is what you want.

Attaching new draft of this patch:

• I agreed with Colin that LongContainingFile was a bad name. I ended up renaming to PersistentLong, and clarified with JavaDoc what its purpose is. I also made it instantiable as a wrapper which holds a persisted long value – this was useful in development of HDFS-3077 in order to hold the "promised epoch" persistent across restarts.

• Instead of introducing StorageErrorReporter, I just moved the error reporting functionality up into Storage instead of NNStorage. It seems like a generally useful thing – in the future we may want to consolidate the error-tracking functionality between the DN and NN using this mechanism, for example. For now, the Storage implementation just logs the errors.

• Change TransferFsImage to now take a Storage instead of NNStorage. This is so that in HDFS-3077, we can download logs into a new JNStorage class.

• Move getFiles() from NNStorage into Storage since it's also generally useful and not NN-specific.

• Some minor refactor in {{TransferFsImage]} to make code more re-usable (also used for edits transfer in HDFS-3077).

While extracting PersistentLong, I also noticed a bug that, if there were an IOE while writing the file, it would still attempt to close the AtomicFileOutputStream. This could cause the incompletely written value to get incorrectly "committed". I added a simple "abort" function for this.

Actually, went back to the original idea of a StorageErrorReporter interface. It makes the dependencies clearer between JournalManager and the Storage, which is nice when trying to unit-test a JournalManager in isolation. Otherwise this patch should be the same.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12533572/hdfs-3190.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

-1 javadoc. The javadoc tool appears to have generated 2 warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2704//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2704//console

This message is automatically generated.

+1, the latest patch looks good to me.

Todd, can you please hold off committing it. I want to take a quick look.

Nice refactoring. +1 for the patch with some minor comments:

1. Some lines are over 80 chars.
2. StorageErrorReporter.java
   • StorageErrorReporter javadoc - can you please add link to JournalManager. I am not very clear on what you mean by "central manager of storage directory state".
   • Also can you please add javadoc description to the methods
3. PersistentLong - PersistentLongFile or AtomicLongFile better name?

Thanks for the review, Suresh. Here's an updated patch.

• Fixed the 80+ long lines.
• Improved javadoc on StorageErrorReporter
• Renamed PersistentLong to PersistentLongFile

I also removed reportErrorOnDirectory from the error reporting interface, in order to simplify it. The only caller was FileJournalManager, and it was easy to change it to just report on the actual files that had an error. Seemed worth simplifying the API here.

+1 for the patch.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12534302/hdfs-3190.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2730//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2730//console

This message is automatically generated.

Thanks for the review, Suresh. I'll commit this momentarily.

Committed to trunk only for now. We can backport the HDFS-3077-related changes en masse when it's all ready in trunk.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12534302/hdfs-3190.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2732//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2732//console

This message is automatically generated.

Integrated in Hadoop-Hdfs-trunk #1094 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1094/)
HDFS-3190. Simple refactors in existing NN code to assist QuorumJournalManager extension. Contributed by Todd Lipcon. (Revision 1356525)

Result = FAILURE
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1356525
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/StorageErrorReporter.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/AtomicFileOutputStream.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/PersistentLongFile.java

Integrated in Hadoop-Mapreduce-trunk #1127 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1127/)
HDFS-3190. Simple refactors in existing NN code to assist QuorumJournalManager extension. Contributed by Todd Lipcon. (Revision 1356525)

Result = SUCCESS
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1356525
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/StorageErrorReporter.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/AtomicFileOutputStream.java
• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/util/PersistentLongFile.java

Backported this to branch-2, since it was causing some conflicts in other backports, and it's a straight refactor.

Integrated in Hadoop-Hdfs-trunk-Commit #2639 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2639/)
Move HDFS-2330 and HDFS-3190 to branch-2 section, since they have been backported from trunk. (Revision 1372605)

Result = SUCCESS
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1372605
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

Integrated in Hadoop-Common-trunk-Commit #2574 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2574/)
Move HDFS-2330 and HDFS-3190 to branch-2 section, since they have been backported from trunk. (Revision 1372605)

Result = SUCCESS
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1372605
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

Integrated in Hadoop-Mapreduce-trunk-Commit #2596 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2596/)
Move HDFS-2330 and HDFS-3190 to branch-2 section, since they have been backported from trunk. (Revision 1372605)

Result = FAILURE
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1372605
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

Integrated in Hadoop-Hdfs-trunk #1135 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1135/)
Move HDFS-2330 and HDFS-3190 to branch-2 section, since they have been backported from trunk. (Revision 1372605)

Result = FAILURE
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1372605
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

Integrated in Hadoop-Mapreduce-trunk #1167 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1167/)
Move HDFS-2330 and HDFS-3190 to branch-2 section, since they have been backported from trunk. (Revision 1372605)

Result = FAILURE
todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1372605
Files :

• /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt