Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-5695

Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 2.2.0
    • Fix Version/s: 2.3.0
    • Component/s: test
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      TestOfflineEditsViewer and OfflineEditsViewerHelper duplicate some of the functionality available in DFSTestUtil. The code can be simplified.

      1. HDFS-5695.001.patch
        21 kB
        Haohui Mai
      2. HDFS-5695.000.patch
        22 kB
        Haohui Mai

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          2d 18h 58m 1 Haohui Mai 23/Dec/13 19:19
          Patch Available Patch Available Resolved Resolved
          10d 22h 28m 1 Jing Zhao 03/Jan/14 17:48
          Resolved Resolved Closed Closed
          52d 3h 8m 1 Arun C Murthy 24/Feb/14 20:56
          Chris Nauroth made changes -
          Link This issue is duplicated by HDFS-5409 [ HDFS-5409 ]
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Arun C Murthy made changes -
          Fix Version/s 2.3.0 [ 12325255 ]
          Fix Version/s 2.4.0 [ 12324588 ]
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1659 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1659/)
          HDFS-5695. Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1659 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1659/ ) HDFS-5695 . Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1634 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1634/)
          HDFS-5695. Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1634 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1634/ ) HDFS-5695 . Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #442 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/442/)
          HDFS-5695. Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #442 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/442/ ) HDFS-5695 . Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Jing Zhao made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 2.4.0 [ 12324588 ]
          Resolution Fixed [ 1 ]
          Hide
          Jing Zhao added a comment -

          I've committed this to trunk and branch-2.

          Show
          Jing Zhao added a comment - I've committed this to trunk and branch-2.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #4955 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4955/)
          HDFS-5695. Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #4955 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4955/ ) HDFS-5695 . Clean up TestOfflineEditsViewer and OfflineEditsViewerHelper. Contributed by Haohui Mai. (jing9: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1555164 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/OfflineEditsViewerHelper.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12621140/HDFS-5695.001.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +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/5812//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5812//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12621140/HDFS-5695.001.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +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/5812//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5812//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          Ha. It looks like both are acceptable spellings. I wouldn't have guessed, especially given that it's spelled differently elsewhere in the same file.

          Anyway, thanks for this cleanup. +1.

          Show
          Colin Patrick McCabe added a comment - Ha. It looks like both are acceptable spellings. I wouldn't have guessed, especially given that it's spelled differently elsewhere in the same file. Anyway, thanks for this cleanup. +1.
          Hide
          Haohui Mai added a comment -

          I'd still like to see "// judgment time" correctly spelled as "judgement time"

          Just did a little bit of googling, it looks like judgment is correct. Please see http://www.dailywritingtips.com/judgement-or-judgment/ for more information.

          Show
          Haohui Mai added a comment - I'd still like to see "// judgment time" correctly spelled as "judgement time" Just did a little bit of googling, it looks like judgment is correct. Please see http://www.dailywritingtips.com/judgement-or-judgment/ for more information.
          Hide
          Colin Patrick McCabe added a comment -

          Thanks, Haohui. I'd still like to see "// judgment time" correctly spelled as "judgement time"

          +1 after that's addressed.

          Show
          Colin Patrick McCabe added a comment - Thanks, Haohui. I'd still like to see "// judgment time" correctly spelled as "judgement time" +1 after that's addressed.
          Hide
          Haohui Mai added a comment -

          Colin Patrick McCabe, I think you have a point. I've updated the patch to keep the test.

          Show
          Haohui Mai added a comment - Colin Patrick McCabe , I think you have a point. I've updated the patch to keep the test.
          Haohui Mai made changes -
          Attachment HDFS-5695.001.patch [ 12621140 ]
          Hide
          Colin Patrick McCabe added a comment -

          Let me give two examples of testStored catching errors that I made. One time, a patch I created broke checksum calculation on the edits. testGenerated passed, since the incorrect checksum was being generated consistently (although not correctly), but testStored pinpointed the problem. Another time, I made a change which accidentally made an optional field required. Of course, testStored found this error since the old editsStored file had been generated by someone without my erroneous patch.

          testStored really is one of the last lines of defense against these kind of problems. The real last line is people trying out the code prior to a release, but I would prefer not to rely on that.

          Show
          Colin Patrick McCabe added a comment - Let me give two examples of testStored catching errors that I made. One time, a patch I created broke checksum calculation on the edits. testGenerated passed, since the incorrect checksum was being generated consistently (although not correctly), but testStored pinpointed the problem. Another time, I made a change which accidentally made an optional field required. Of course, testStored found this error since the old editsStored file had been generated by someone without my erroneous patch. testStored really is one of the last lines of defense against these kind of problems. The real last line is people trying out the code prior to a release, but I would prefer not to rely on that.
          Hide
          Colin Patrick McCabe added a comment -

          Because the edit log generated by testStored was (usually) generated on a different computer than the computer running the JUnit test, testStored can and does catch problems that testGenerated does not. I have seen this happen in the past. I agree that it is somewhat annoying that test-patch.sh doesn't currently support binary diffs, which sometimes leads to this file not getting updated when it should, but that seems like something to fix in test-patch.sh, not by removing tests.

          For now I think it might be a good time to remove it. We can add a rigorous test suite to test the compatibility of edit logs if it is needed.

          If you can think of any test that would have the same coverage as testStored, we could think about replacing testStored with that. I don't think we should remove testStored until we have added test that could replace it.

          Show
          Colin Patrick McCabe added a comment - Because the edit log generated by testStored was (usually) generated on a different computer than the computer running the JUnit test, testStored can and does catch problems that testGenerated does not. I have seen this happen in the past. I agree that it is somewhat annoying that test-patch.sh doesn't currently support binary diffs, which sometimes leads to this file not getting updated when it should, but that seems like something to fix in test-patch.sh, not by removing tests. For now I think it might be a good time to remove it. We can add a rigorous test suite to test the compatibility of edit logs if it is needed. If you can think of any test that would have the same coverage as testStored, we could think about replacing testStored with that. I don't think we should remove testStored until we have added test that could replace it.
          Hide
          Haohui Mai added a comment -

          I don't understand why the new version lacks TestOfflineEditsViewer#testStored. That seems like it would reduce test coverage.

          testStored loads the binary / xml format of the edit logs and checks whether the viewer can (1) load the log, and (2) the edit logs contain all desired ops. It goes through the exact code path of testGenerated.

          Ideally it is a test for backward compatibility, but in practice we (as developers) just replace the stored edit logs with the one generated by testGenerated. This practice has been followed by snapshots, DN cache, and ACLs. It seems to me that the test has little value but only requires additional work (e.g., HDFS-5636).

          For now I think it might be a good time to remove it. We can add a rigorous test suite to test the compatibility of edit logs if it is needed.

          Show
          Haohui Mai added a comment - I don't understand why the new version lacks TestOfflineEditsViewer#testStored. That seems like it would reduce test coverage. testStored loads the binary / xml format of the edit logs and checks whether the viewer can (1) load the log, and (2) the edit logs contain all desired ops. It goes through the exact code path of testGenerated . Ideally it is a test for backward compatibility, but in practice we (as developers) just replace the stored edit logs with the one generated by testGenerated . This practice has been followed by snapshots, DN cache, and ACLs. It seems to me that the test has little value but only requires additional work (e.g., HDFS-5636 ). For now I think it might be a good time to remove it. We can add a rigorous test suite to test the compatibility of edit logs if it is needed.
          Hide
          Colin Patrick McCabe added a comment -
              // judgment time
          

          while you're improving this, can you fix the spelling here?

          I don't understand why the new version lacks TestOfflineEditsViewer#testStored. That seems like it would reduce test coverage.

          Show
          Colin Patrick McCabe added a comment - // judgment time while you're improving this, can you fix the spelling here? I don't understand why the new version lacks TestOfflineEditsViewer#testStored . That seems like it would reduce test coverage.
          Chris Nauroth made changes -
          Link This issue is duplicated by HDFS-5409 [ HDFS-5409 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12620242/HDFS-5695.000.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          +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/5789//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5789//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12620242/HDFS-5695.000.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +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/5789//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5789//console This message is automatically generated.
          Chris Nauroth made changes -
          Component/s test [ 12312916 ]
          Hadoop Flags Reviewed [ 10343 ]
          Affects Version/s 2.2.0 [ 12325049 ]
          Affects Version/s 3.0.0 [ 12320356 ]
          Target Version/s 3.0.0, 2.4.0 [ 12320356, 12324588 ]
          Hide
          Chris Nauroth added a comment -

          +1 for the patch, pending Jenkins. I assume the intention would be to delete editsStored and editsStored.xml from the svn repo too, right? I'm going to hold off on committing for a little while in case anyone out there has a reason to keep the tests as they are now.

          Show
          Chris Nauroth added a comment - +1 for the patch, pending Jenkins. I assume the intention would be to delete editsStored and editsStored.xml from the svn repo too, right? I'm going to hold off on committing for a little while in case anyone out there has a reason to keep the tests as they are now.
          Haohui Mai made changes -
          Link This issue is depended upon by HDFS-5618 [ HDFS-5618 ]
          Haohui Mai made changes -
          Link This issue relates to HDFS-5636 [ HDFS-5636 ]
          Haohui Mai made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Haohui Mai made changes -
          Field Original Value New Value
          Attachment HDFS-5695.000.patch [ 12620242 ]
          Haohui Mai created issue -

            People

            • Assignee:
              Haohui Mai
              Reporter:
              Haohui Mai
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development