Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.4.5, 3.5.0
    • Fix Version/s: 3.4.6, 3.5.0
    • Component/s: tests
    • Labels:
      None
    1. ZOOKEEPER-1849.patch
      11 kB
      Germán Blanco
    2. ZOOKEEPER-1849.patch
      11 kB
      Germán Blanco
    3. ZOOKEEPER-1849.patch
      4 kB
      Germán Blanco

      Activity

      Hide
      Germán Blanco added a comment -

      It is a different test case that is failing for me at the moment, but it could be related.
      I am arriving to the conclusion that some of the test cases leave running threads and therefore CPU and memory resources allocated, and that makes it more likely for subsequent test cases to fail.
      Adding a check for used memory and number of threads before each test case seems to confirm this.
      It seems e.g. that MaxCnxnsTest doesn't close SocketChannels after use.
      I will upload a patch that closes the SocketChannels and adds a log of free memory and number of running threads.

      Show
      Germán Blanco added a comment - It is a different test case that is failing for me at the moment, but it could be related. I am arriving to the conclusion that some of the test cases leave running threads and therefore CPU and memory resources allocated, and that makes it more likely for subsequent test cases to fail. Adding a check for used memory and number of threads before each test case seems to confirm this. It seems e.g. that MaxCnxnsTest doesn't close SocketChannels after use. I will upload a patch that closes the SocketChannels and adds a log of free memory and number of running threads.
      Hide
      Germán Blanco added a comment -

      It seems that PurgeTxnTest creates a ZooKeeperServer that is never shutdown.

      Show
      Germán Blanco added a comment - It seems that PurgeTxnTest creates a ZooKeeperServer that is never shutdown.
      Hide
      Germán Blanco added a comment -

      In LoadFromLogTest some ZooKeeperServers are shutdown, but others are not.

      Show
      Germán Blanco added a comment - In LoadFromLogTest some ZooKeeperServers are shutdown, but others are not.
      Hide
      Germán Blanco added a comment -

      testBindByAddress doesn't shutdown ZooKeeperServer

      Show
      Germán Blanco added a comment - testBindByAddress doesn't shutdown ZooKeeperServer
      Hide
      Germán Blanco added a comment -

      The attached patch shuts down a few ZooKeeperServers and SocketChannels that were left running in some tests.
      It also adds a log of used memory and running threads in the start of every ZKTestCase.
      In my Windows box, when running together with the fix of FLETest, all java tests are executed succesfully with this fix.
      Still, there seem to be other tests that leave running instances without closing.

      Show
      Germán Blanco added a comment - The attached patch shuts down a few ZooKeeperServers and SocketChannels that were left running in some tests. It also adds a log of used memory and running threads in the start of every ZKTestCase. In my Windows box, when running together with the fix of FLETest, all java tests are executed succesfully with this fix. Still, there seem to be other tests that leave running instances without closing.
      Hide
      Germán Blanco added a comment -

      This patch can be applied to both trunk and branch 3.4, so QA is expected to be succesful.

      Show
      Germán Blanco added a comment - This patch can be applied to both trunk and branch 3.4, so QA is expected to be succesful.
      Hide
      Hadoop QA added a comment -

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

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

      +1 tests included. The patch appears to include 15 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 (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 core unit tests.

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

      Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1848//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1848//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1848//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/12619275/ZOOKEEPER-1849.patch against trunk revision 1551624. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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 (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 core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1848//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1848//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1848//console This message is automatically generated.
      Hide
      Flavio Junqueira added a comment -

      Thanks, Germán. Would you mind posting this on the review board so that it is easier to check the position of the shutdown calls? It looks good from a high level, but I'd like to have a closer look. Also, could you please check the format of the parts of the code this patch is touching, in particular LoadFromLogTest? The indentation does not seem right.

      Show
      Flavio Junqueira added a comment - Thanks, Germán. Would you mind posting this on the review board so that it is easier to check the position of the shutdown calls? It looks good from a high level, but I'd like to have a closer look. Also, could you please check the format of the parts of the code this patch is touching, in particular LoadFromLogTest? The indentation does not seem right.
      Hide
      Germán Blanco added a comment -

      Thank you for taking a look.
      Here is the review: https://reviews.apache.org/r/16349
      Sorry but I didn't find the problem in LoadFromLogTest.

      Show
      Germán Blanco added a comment - Thank you for taking a look. Here is the review: https://reviews.apache.org/r/16349 Sorry but I didn't find the problem in LoadFromLogTest.
      Hide
      Germán Blanco added a comment -

      Now I see the problem. The thing is that there are spaces instead of tabs in the lines around the change ... how should we handle this?

      • update the lines around the change (as far as I know it could be all lines in the file)?
      • use the same indentation that the lines around, even if it doesn't follow the convention?
      • or leave it as it is now?
      Show
      Germán Blanco added a comment - Now I see the problem. The thing is that there are spaces instead of tabs in the lines around the change ... how should we handle this? update the lines around the change (as far as I know it could be all lines in the file)? use the same indentation that the lines around, even if it doesn't follow the convention? or leave it as it is now?
      Hide
      Flavio Junqueira added a comment -

      Reformatting the whole file is typically a bad idea because the reformat changes get mixed with the changes of the patch and it becomes to difficult to understand what is going on. Some people are more strict than I tend to be with this, and I'm personally ok if you do a small number of reformat changes around the original changes of the patch. But, someone can always come and say that this is not a reformatting issue and -1 it.

      Show
      Flavio Junqueira added a comment - Reformatting the whole file is typically a bad idea because the reformat changes get mixed with the changes of the patch and it becomes to difficult to understand what is going on. Some people are more strict than I tend to be with this, and I'm personally ok if you do a small number of reformat changes around the original changes of the patch. But, someone can always come and say that this is not a reformatting issue and -1 it.
      Hide
      Flavio Junqueira added a comment -

      There is only one test case that fails for me now in NioNettySuiteHammerTest:

      Testcase: testHammer took 149.091 sec
      	FAILED
      waiting for server up
      junit.framework.AssertionFailedError: waiting for server up
      	at org.apache.zookeeper.test.QuorumBase.startServers(QuorumBase.java:188)
      	at org.apache.zookeeper.test.QuorumBase.startServers(QuorumBase.java:113)
      	at org.apache.zookeeper.test.AsyncHammerTest.restart(AsyncHammerTest.java:61)
      	at org.apache.zookeeper.test.AsyncHammerTest.testHammer(AsyncHammerTest.java:193)
      	at org.apache.zookeeper.JUnit4ZKTestRunner$LoggedInvokeMethod.evaluate(JUnit4ZKTestRunner.java:52)
      
      Testcase: testObserversHammer took 25.375 sec
      

      I think you said it doesn't fail for you, is it right, Germán?

      Show
      Flavio Junqueira added a comment - There is only one test case that fails for me now in NioNettySuiteHammerTest: Testcase: testHammer took 149.091 sec FAILED waiting for server up junit.framework.AssertionFailedError: waiting for server up at org.apache.zookeeper.test.QuorumBase.startServers(QuorumBase.java:188) at org.apache.zookeeper.test.QuorumBase.startServers(QuorumBase.java:113) at org.apache.zookeeper.test.AsyncHammerTest.restart(AsyncHammerTest.java:61) at org.apache.zookeeper.test.AsyncHammerTest.testHammer(AsyncHammerTest.java:193) at org.apache.zookeeper.JUnit4ZKTestRunner$LoggedInvokeMethod.evaluate(JUnit4ZKTestRunner.java:52) Testcase: testObserversHammer took 25.375 sec I think you said it doesn't fail for you, is it right, Germán?
      Hide
      Germán Blanco added a comment -

      Added shutdowns in a few more places.
      I have reformatted the lines around the indentation problem, they are around 100 lines. Then I decided to rollback the indentation changes because with them, the patch doesn't work for trunk any more.

      Show
      Germán Blanco added a comment - Added shutdowns in a few more places. I have reformatted the lines around the indentation problem, they are around 100 lines. Then I decided to rollback the indentation changes because with them, the patch doesn't work for trunk any more.
      Hide
      Germán Blanco added a comment -

      No, it doesn't fail for me and I have run the tests many times now.
      But that makes me think, maybe all these shutdowns are not required after all .

      Show
      Germán Blanco added a comment - No, it doesn't fail for me and I have run the tests many times now. But that makes me think, maybe all these shutdowns are not required after all .
      Hide
      Germán Blanco added a comment -

      Could you please upload the logs?

      Show
      Germán Blanco added a comment - Could you please upload the logs?
      Hide
      Germán Blanco added a comment -

      Not that I have a lot of confidence now, but in any case, just to discard possibilities out, could you please also try the latest version of the patch?

      Show
      Germán Blanco added a comment - Not that I have a lot of confidence now, but in any case, just to discard possibilities out, could you please also try the latest version of the 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/12619482/ZOOKEEPER-1849.patch
      against trunk revision 1551624.

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

      +1 tests included. The patch appears to include 42 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 (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 core unit tests.

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

      Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1849//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1849//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1849//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/12619482/ZOOKEEPER-1849.patch against trunk revision 1551624. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 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 (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 core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1849//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1849//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1849//console This message is automatically generated.
      Hide
      Flavio Junqueira added a comment -

      All tests pass for me with the new patch, very nice! Let me run tests a few times more to make sure.

      Show
      Flavio Junqueira added a comment - All tests pass for me with the new patch, very nice! Let me run tests a few times more to make sure.
      Hide
      Flavio Junqueira added a comment -

      +1, lgtm

      Show
      Flavio Junqueira added a comment - +1, lgtm
      Hide
      Flavio Junqueira added a comment -

      B3.4: Committed revision 1552447.

      The current patch doesn't apply cleanly to trunk, Germán Blanco.

      Show
      Flavio Junqueira added a comment - B3.4: Committed revision 1552447. The current patch doesn't apply cleanly to trunk, Germán Blanco .
      Hide
      Germán Blanco added a comment -

      The attached patch has been generated against current trunk.

      Show
      Germán Blanco added a comment - The attached patch has been generated against current trunk.
      Hide
      Hadoop QA added a comment -

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

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

      +1 tests included. The patch appears to include 42 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 (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 core unit tests.

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

      Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1853//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1853//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
      Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1853//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/12619776/ZOOKEEPER-1849.patch against trunk revision 1552469. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 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 (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 core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1853//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1853//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1853//console This message is automatically generated.
      Hide
      Flavio Junqueira added a comment -

      Trunk: Committed revision 1552946.

      Show
      Flavio Junqueira added a comment - Trunk: Committed revision 1552946.
      Hide
      Hudson added a comment -

      SUCCESS: Integrated in ZooKeeper-trunk #2161 (See https://builds.apache.org/job/ZooKeeper-trunk/2161/)
      ZOOKEEPER-1849. Need to properly tear down tests in various cases
      (Germán Blanco via fpj) (fpj: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1552946)

      • /zookeeper/trunk/CHANGES.txt
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/JUnit4ZKTestRunner.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/CRCTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ACLCountTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ACLTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ClientPortBindTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/InvalidSnapshotTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MaxCnxnsTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/OOMTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RecoveryTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RepeatStartupTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RestoreCommittedLogTest.java
      • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SessionTest.java
      Show
      Hudson added a comment - SUCCESS: Integrated in ZooKeeper-trunk #2161 (See https://builds.apache.org/job/ZooKeeper-trunk/2161/ ) ZOOKEEPER-1849 . Need to properly tear down tests in various cases (Germán Blanco via fpj) (fpj: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1552946 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/test/org/apache/zookeeper/JUnit4ZKTestRunner.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/CRCTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ACLCountTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ACLTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/ClientPortBindTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/InvalidSnapshotTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MaxCnxnsTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/OOMTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/PurgeTxnTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RecoveryTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RepeatStartupTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/RestoreCommittedLogTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SessionTest.java
      Hide
      Flavio Junqueira added a comment -

      Closing issues after releasing 3.4.6.

      Show
      Flavio Junqueira added a comment - Closing issues after releasing 3.4.6.

        People

        • Assignee:
          Germán Blanco
          Reporter:
          Germán Blanco
        • Votes:
          0 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development