Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-9058

deleteSnapshot() call may be skipped in TestFlushSnapshotFromClient tests

    Details

    • Type: Test
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      At the end of each test, we have this call:

      admin.deleteSnapshot(snapshotName);

      However it is not placed in finally block. When assertion in earlier part of the test fails (See [1]), snapshot cleanup would be skipped, leading to SnapshotTestingUtils.assertNoSnapshots() failing for subsequent tests.

      [1]: https://builds.apache.org/job/hbase-0.95-on-hadoop2/197/testReport/org.apache.hadoop.hbase.snapshot/TestFlushSnapshotFromClient/testFlushCreateListDestroy/

      1. 9058-v2.patch
        3 kB
        Ted Yu
      2. 9058.patch
        3 kB
        Ted Yu
      3. 9058.patch
        3 kB
        Ted Yu

        Activity

        Hide
        mbertozzi Matteo Bertozzi added a comment -

        instead of adding a finally block in every test, the foreach snapshot delete should be added to the tearDown() method.. since every test is doing that anyway.

        Show
        mbertozzi Matteo Bertozzi added a comment - instead of adding a finally block in every test, the foreach snapshot delete should be added to the tearDown() method.. since every test is doing that anyway.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        I thought of that approach.
        Currently the snapshot name changes with each test, e.g.:

            String snapshotName = "flushSnapshotCreateListDestroy";
        

        Shall we introduce a class variable which records the snapshot name ?

        Show
        yuzhihong@gmail.com Ted Yu added a comment - I thought of that approach. Currently the snapshot name changes with each test, e.g.: String snapshotName = "flushSnapshotCreateListDestroy" ; Shall we introduce a class variable which records the snapshot name ?
        Hide
        mbertozzi Matteo Bertozzi added a comment -

        nope, just for each admin.listSnapshot() deleteSnapshot() should be fine, since each test assert that no snapshot.
        There're other tests that does this

        Show
        mbertozzi Matteo Bertozzi added a comment - nope, just for each admin.listSnapshot() deleteSnapshot() should be fine, since each test assert that no snapshot. There're other tests that does this
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Patch removes snapshots directory after each test.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Patch removes snapshots directory after each test.
        Hide
        mbertozzi Matteo Bertozzi added a comment -

        wrong attachment...

        Show
        mbertozzi Matteo Bertozzi added a comment - wrong attachment...
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Attaching right patch.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Attaching right patch.
        Hide
        hadoopqa Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6495//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12594540/9058.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6495//console This message is automatically generated.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Rebase.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Rebase.
        Hide
        mbertozzi Matteo Bertozzi added a comment -

        m.. removing the directory works, for now. but it relies on the fact that we don't keep anything in-memory to track the snapshots.

        Show
        mbertozzi Matteo Bertozzi added a comment - m.. removing the directory works, for now. but it relies on the fact that we don't keep anything in-memory to track the snapshots.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Same approach (removing the directory) is used in TestSnapshotFromMaster.java

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Same approach (removing the directory) is used in TestSnapshotFromMaster.java
        Hide
        mbertozzi Matteo Bertozzi added a comment -

        it doesn't mean that is the logically correct one
        you're removing a dir with a running hbase, not just on hbase shutdown.
        This means that the snapshot stuff must work even if the dir is not there,
        and that the snapshot code can not rely on .snapshot startup initialization.

        I prefer a list/delete way

        for (SnapshotDescription snapshot: admin.listSnapshots()) {
           admin.deleteSnapshot(snapshot.getName());
        }
        

        but do what you prefer, as you said we have already some code like this

        Show
        mbertozzi Matteo Bertozzi added a comment - it doesn't mean that is the logically correct one you're removing a dir with a running hbase, not just on hbase shutdown. This means that the snapshot stuff must work even if the dir is not there, and that the snapshot code can not rely on .snapshot startup initialization. I prefer a list/delete way for (SnapshotDescription snapshot: admin.listSnapshots()) { admin.deleteSnapshot(snapshot.getName()); } but do what you prefer, as you said we have already some code like this
        Hide
        hadoopqa Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +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 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 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.client.TestSnapshotFromClient

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12594541/9058.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestSnapshotFromClient Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6496//console This message is automatically generated.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Patch v2 iterates through snapshots and deletes each snapshot in tearDown().

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Patch v2 iterates through snapshots and deletes each snapshot in tearDown().
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        From https://builds.apache.org/job/PreCommit-HBASE-Build/6499/console :

        At revision 1507709.
        HBASE-9058 patch is being downloaded at Sat Jul 27 20:21:50 UTC 2013 from
        http://issues.apache.org/jira/secure/attachment/12594557/9058-v2.patch
        ...
        Running org.apache.hadoop.hbase.snapshot.TestFlushSnapshotFromClient
        Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 112.416 sec
        
        Show
        yuzhihong@gmail.com Ted Yu added a comment - From https://builds.apache.org/job/PreCommit-HBASE-Build/6499/console : At revision 1507709. HBASE-9058 patch is being downloaded at Sat Jul 27 20:21:50 UTC 2013 from http: //issues.apache.org/jira/secure/attachment/12594557/9058-v2.patch ... Running org.apache.hadoop.hbase.snapshot.TestFlushSnapshotFromClient Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 112.416 sec
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Ran the patch against hadoop 2 which passed as well.

        Integrated to trunk.

        Thanks for the review, Matteo.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Ran the patch against hadoop 2 which passed as well. Integrated to trunk. Thanks for the review, Matteo.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Don't know why there wasn't post back from Jenkins.

        Both builds passed:
        https://builds.apache.org/job/HBase-TRUNK/4305/
        https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/637/

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Don't know why there wasn't post back from Jenkins. Both builds passed: https://builds.apache.org/job/HBase-TRUNK/4305/ https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/637/

          People

          • Assignee:
            yuzhihong@gmail.com Ted Yu
            Reporter:
            yuzhihong@gmail.com Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development