HBase
  1. HBase
  2. HBASE-9058

deleteSnapshot() call may be skipped in TestFlushSnapshotFromClient tests

    Details

    • Type: Test Test
    • Status: Resolved
    • Priority: Minor 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.patch
        3 kB
        Ted Yu
      2. 9058.patch
        3 kB
        Ted Yu
      3. 9058-v2.patch
        3 kB
        Ted Yu

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        Ted Yu added a comment -

        Patch removes snapshots directory after each test.

        Show
        Ted Yu added a comment - Patch removes snapshots directory after each test.
        Hide
        Matteo Bertozzi added a comment -

        wrong attachment...

        Show
        Matteo Bertozzi added a comment - wrong attachment...
        Hide
        Ted Yu added a comment -

        Attaching right patch.

        Show
        Ted Yu added a comment - Attaching right patch.
        Hide
        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
        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
        Ted Yu added a comment -

        Rebase.

        Show
        Ted Yu added a comment - Rebase.
        Hide
        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
        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
        Ted Yu added a comment -

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

        Show
        Ted Yu added a comment - Same approach (removing the directory) is used in TestSnapshotFromMaster.java
        Hide
        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
        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
        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
        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
        Ted Yu added a comment -

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

        Show
        Ted Yu added a comment - Patch v2 iterates through snapshots and deletes each snapshot in tearDown().
        Hide
        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
        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
        Ted Yu added a comment -

        Ran the patch against hadoop 2 which passed as well.

        Integrated to trunk.

        Thanks for the review, Matteo.

        Show
        Ted Yu added a comment - Ran the patch against hadoop 2 which passed as well. Integrated to trunk. Thanks for the review, Matteo.
        Hide
        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
        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:
            Ted Yu
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development