Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.0
    • Component/s: contrib-bookkeeper
    • Labels:
      None

      Description

      The test case testAsyncCreateClose is badly broken. I was wondering why all the unit tests are passing inspite of having found so many different problems with LedgerManagementProcessor.

      There is a big try-catch block sitting in the test case that catches all exception, prints their stack trace, and exits, thereby allowing the test to pass. In general, unit tests shouldnt catch exceptions unless it is something you are expecting that will happen.

      Another problem is that the same ControlObject is used for synchronization throughout. Since we already have the problem of callbacks being called multiple times (ZOOKEEPER-502), notify() on the control object is called too many times, resulting in the unit test not waiting for certain callbacks.

      Thus the test never waits for the asyncOpenLedger() to finish, and hence still succeeds. I believe asyncOpenLedger() has never worked right.

      1. ZOOKEEPER-505.1.patch
        2 kB
        Utkarsh Srivastava

        Issue Links

          Activity

          Hide
          Utkarsh Srivastava added a comment -

          Fix the unit test to not catch exceptions
          Actually wait for openCallback to be called by using a new control object

          Show
          Utkarsh Srivastava added a comment - Fix the unit test to not catch exceptions Actually wait for openCallback to be called by using a new control object
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12415968/ZOOKEEPER-505.1.patch
          against trunk revision 802188.

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

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

          +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 warnings.

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/181/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/181/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/181/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/12415968/ZOOKEEPER-505.1.patch against trunk revision 802188. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/181/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/181/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/181/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          Great catch, Utkarsh! I noticed this problem a while ago in the ReadWrite tests and fixed by changing the catch blocks to the following:

          catch (BKException e) {
                LOG.error("Test failed", e);
                fail("Test failed due to BookKeeper exception");
          }
          

          Clearly we forgot to implement the change in all places. Also, we shouldn't have calls to printStackTrace. Now I wonder if it is best to remove the catch block as you propose or to catch and fail as I propose above. I don't know the exact semantics of junit with respect to exceptions, but recently I've seen a case that a test was throwing an exception and not catching it, and was still passing.

          Show
          Flavio Junqueira added a comment - Great catch, Utkarsh! I noticed this problem a while ago in the ReadWrite tests and fixed by changing the catch blocks to the following: catch (BKException e) { LOG.error("Test failed", e); fail("Test failed due to BookKeeper exception"); } Clearly we forgot to implement the change in all places. Also, we shouldn't have calls to printStackTrace. Now I wonder if it is best to remove the catch block as you propose or to catch and fail as I propose above. I don't know the exact semantics of junit with respect to exceptions, but recently I've seen a case that a test was throwing an exception and not catching it, and was still passing.
          Hide
          Flavio Junqueira added a comment -

          Modifications under discussion.

          Show
          Flavio Junqueira added a comment - Modifications under discussion.
          Hide
          Mahadev konar added a comment -

          committed in ZOOKEEPER-507

          Show
          Mahadev konar added a comment - committed in ZOOKEEPER-507

            People

            • Assignee:
              Utkarsh Srivastava
              Reporter:
              Utkarsh Srivastava
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development