ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1833 fix windows build
  3. ZOOKEEPER-1459

Standalone ZooKeeperServer is not closing the transaction log files on shutdown

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.4.0
    • Fix Version/s: 3.4.6, 3.5.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change

      Description

      When shutdown the standalone ZK server, its only clearing the zkdatabase and not closing the transaction log streams. When tries to delete the temporary files in unit tests on windows, its failing.
      ZooKeeperServer.java

              if (zkDb != null) {
                  zkDb.clear();
              }
      

      Suggestion to close the zkDb as follows, this inturn will take care transaction logs:

              if (zkDb != null) {
                  zkDb.clear();
                  try {
                      zkDb.close();
                  } catch (IOException ie) {
                      LOG.warn("Error closing logs ", ie);
                  }
              }
      
      1. ZOOKEEPER-1459.patch
        3 kB
        Rakesh R
      2. ZOOKEEPER-1459.patch
        4 kB
        Rakesh R
      3. ZOOKEEPER-1459.patch
        4 kB
        David Lao
      4. ZOOKEEPER-1459.patch
        1 kB
        David Lao
      5. ZOOKEEPER-1459.patch
        4 kB
        Rakesh R
      6. ZOOKEEPER-1459.patch
        1 kB
        Rakesh R
      7. ZOOKEEPER-1459.patch
        1 kB
        Rakesh R
      8. ZOOKEEPER-1459.patch
        0.6 kB
        Rakesh R
      9. ZOOKEEPER-1459-branch-3_4.patch
        3 kB
        Rakesh R
      10. ZOOKEEPER-1459-branch-3_4.patch
        4 kB
        Rakesh R

        Activity

        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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 failed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1053//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1053//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1053//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/12525047/ZOOKEEPER-1459.patch against trunk revision 1331246. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1053//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1053//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1053//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        Canelling the patch, since it has few correction as per the report. Please help me to review the cause and suggested fix. I will try to upload a clean patch. Thanks.

        Show
        Rakesh R added a comment - Canelling the patch, since it has few correction as per the report. Please help me to review the cause and suggested fix. I will try to upload a clean patch. Thanks.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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 failed core unit tests.

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

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1636//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1636//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1636//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/12525047/ZOOKEEPER-1459.patch against trunk revision 1529013. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1636//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1636//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1636//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        Unless we see a working patch from Rakesh R I'm not sure this belongs in the critical list for 3.4.6.

        Show
        Camille Fournier added a comment - Unless we see a working patch from Rakesh R I'm not sure this belongs in the critical list for 3.4.6.
        Hide
        Rakesh R added a comment -

        Thanks Camille Fournier for the interest. I had wrongly closed the ZKDatabase in my first patch. Here it was failing in SyncRequestProcessor thread. Please have a look at the latest patch.

        Show
        Rakesh R added a comment - Thanks Camille Fournier for the interest. I had wrongly closed the ZKDatabase in my first patch. Here it was failing in SyncRequestProcessor thread. Please have a look at the latest 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/12606716/ZOOKEEPER-1459.patch
        against trunk revision 1529013.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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/1638//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1638//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1638//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/12606716/ZOOKEEPER-1459.patch against trunk revision 1529013. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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/1638//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1638//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1638//console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        I'm not sure what you mean with "leaking" in the description here, Rakesh R. In fact, I was hoping that this patch would solve the problem of deleting temporary files in unit tests on windows, but it didn't.

        I agree with Camille Fournier, this is not a blocker for 3.4.6, but it is ok to have it if we can get a fix.

        Show
        Flavio Junqueira added a comment - I'm not sure what you mean with "leaking" in the description here, Rakesh R . In fact, I was hoping that this patch would solve the problem of deleting temporary files in unit tests on windows, but it didn't. I agree with Camille Fournier , this is not a blocker for 3.4.6, but it is ok to have it if we can get a fix.
        Hide
        Rakesh R added a comment -

        Flavio Junqueira exactly unit test case is failing in windows. I've modified the defect description and attached latest patch to fix the problem. Please see.

        Show
        Rakesh R added a comment - Flavio Junqueira exactly unit test case is failing in windows. I've modified the defect description and attached latest patch to fix the problem. Please see.
        Hide
        Hadoop QA added a comment -

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

        +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 (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/1667//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1667//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1667//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/12607524/ZOOKEEPER-1459.patch against trunk revision 1530166. +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 (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/1667//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1667//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1667//console This message is automatically generated.
        Hide
        David Lao added a comment -

        Hi. The Oct 8 patch does not actually close the transaction log. The log need to be closed explicitly via the FileTxnSnapLog handle that's passed into the ZooKeeperServer when bootstrapped through runFromConfig. I'm attach a patch generated from release-3.4.5 source that fixes it.

        Please run a QA pass on it. Thanks.

        Show
        David Lao added a comment - Hi. The Oct 8 patch does not actually close the transaction log. The log need to be closed explicitly via the FileTxnSnapLog handle that's passed into the ZooKeeperServer when bootstrapped through runFromConfig. I'm attach a patch generated from release-3.4.5 source that fixes it. Please run a QA pass on it. Thanks.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1723//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/12610271/ZOOKEEPER-1459.patch against trunk revision 1535491. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1723//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        Hi David Lao, Thanks for the review. Hope you are referring to the Oct 9th patch. Here I could see one case, it will fail to close the snapLog only when zkServer.shutdown(); throws an exception, are you pointing me to the same?

        Show
        Rakesh R added a comment - Hi David Lao , Thanks for the review. Hope you are referring to the Oct 9th patch. Here I could see one case, it will fail to close the snapLog only when zkServer.shutdown(); throws an exception, are you pointing me to the same?
        Hide
        David Lao added a comment -

        Add patch for trunk so QA can build. In ZooKeeperServer, the transaction log handle is txnLogFactory rather than zkDb, so that needs to be closed explicitly in order for deleting temp directory to work on Windows.

        Show
        David Lao added a comment - Add patch for trunk so QA can build. In ZooKeeperServer, the transaction log handle is txnLogFactory rather than zkDb, so that needs to be closed explicitly in order for deleting temp directory to work on Windows.
        Hide
        Hadoop QA added a comment -

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

        +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-ZOOKEEPER-Build/1724//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/12610294/ZOOKEEPER-1459.patch against trunk revision 1535491. +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-ZOOKEEPER-Build/1724//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        In ZooKeeperServer, the transaction log handle is txnLogFactory rather than zkDb

        Please see ServerCnxnFactory, here startup of the serverCnxn would create ZKDatabase by giving the txnLogFactory. I feel only if there is an exception in ZooKeeperServer startup, it can leave the txnLog as open. Attached latest patch where closed stream in finally block.

                zks.startdata();
                zks.startup();
        

        Secondly, In the patch you are trying to close the txnLog in zk.shutdown. Initially I had tried similar approach(please refer my first patch) and faced few issues in FollowerZooKeeperServer quorum shutdown. If we look at QuorumPeer shutdown pattern, it is closing the ZooKeeperDatabase after shutting down the quorum at the last step. But with this patch, its closing the transactionlog within the shutdown.

        ZooKeeperServer.java
        + if (txnLogFactory != null)
        +              txnLogFactory.close();
        
        Show
        Rakesh R added a comment - In ZooKeeperServer, the transaction log handle is txnLogFactory rather than zkDb Please see ServerCnxnFactory, here startup of the serverCnxn would create ZKDatabase by giving the txnLogFactory. I feel only if there is an exception in ZooKeeperServer startup, it can leave the txnLog as open. Attached latest patch where closed stream in finally block. zks.startdata(); zks.startup(); Secondly, In the patch you are trying to close the txnLog in zk.shutdown. Initially I had tried similar approach(please refer my first patch) and faced few issues in FollowerZooKeeperServer quorum shutdown. If we look at QuorumPeer shutdown pattern, it is closing the ZooKeeperDatabase after shutting down the quorum at the last step. But with this patch, its closing the transactionlog within the shutdown. ZooKeeperServer.java + if (txnLogFactory != null ) + txnLogFactory.close();
        Hide
        Hadoop QA added a comment -

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

        +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 (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/1725//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1725//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1725//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/12610310/ZOOKEEPER-1459.patch against trunk revision 1535491. +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 (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/1725//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1725//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1725//console This message is automatically generated.
        Hide
        David Lao added a comment -

        Bear with me a bit since I'm new to the code base.. but I don't see ServerCnxnFactory closing the transaction log anywhere. My understanding is that ServerCnxnFactory deals only with socket connectivities. To your second point I agree the log closing is better done outside the server in runFromConfig. I've verified the patch works on by Windows box.

        Show
        David Lao added a comment - Bear with me a bit since I'm new to the code base.. but I don't see ServerCnxnFactory closing the transaction log anywhere. My understanding is that ServerCnxnFactory deals only with socket connectivities. To your second point I agree the log closing is better done outside the server in runFromConfig. I've verified the patch works on by Windows box.
        Hide
        Flavio Junqueira added a comment -

        Both patches, from Rakesh R and David Lao, hang on org.apache.zookeeper.server.ZxidRolloverTest for me.

        David Lao is right that a lot of tests fail on Windows because we don't close the txn log. I don't observe the same on unix and it seems to be due to the semantics of file deletion.

        David Lao, when you generate patches, the patch root should be the project root (e.g., src/java....).

        Show
        Flavio Junqueira added a comment - Both patches, from Rakesh R and David Lao , hang on org.apache.zookeeper.server.ZxidRolloverTest for me. David Lao is right that a lot of tests fail on Windows because we don't close the txn log. I don't observe the same on unix and it seems to be due to the semantics of file deletion. David Lao , when you generate patches, the patch root should be the project root (e.g., src/java....).
        Hide
        Rakesh R added a comment -

        David Lao You are right, ServerCnxnFactory is not closing the transaction log. Sorry for the confusion. I had commented by looking at your patch, its trying to fix the transaction log separately in ZooKeeperServer. This is not required as ZKDatabase is holding the reference to txnLogFactory and zkDatabase will close transaction log also.

        To your second point I agree the log closing is better done outside the server in runFromConfig. I've verified the patch works on by Windows box.

        Thanks David Lao for the testing effort.

        Both patches, from Rakesh R and David Lao, hang on org.apache.zookeeper.server.ZxidRolloverTest for me.

        Flavio Junqueira, Thanks again for the reviews. In the latest patch I tried to fix the problem in ZooKeeperServerMain.java class. One thing I'm not able to understand is, why this fix is affecting ZxidRolloverTest testcases. Because am not seeing any relation ship between ZxidRolloverTest and transaction log closure in ZooKeeperServerMain. I hope you are looking at the latest patch which I attached on "Last Friday 17:244"
        For understanding the problem, it would be good if you can share logs, threaddumps etc. Is this consistently failiing and which test case in ZxidRolloverTest ?

        Show
        Rakesh R added a comment - David Lao You are right, ServerCnxnFactory is not closing the transaction log. Sorry for the confusion. I had commented by looking at your patch, its trying to fix the transaction log separately in ZooKeeperServer. This is not required as ZKDatabase is holding the reference to txnLogFactory and zkDatabase will close transaction log also. To your second point I agree the log closing is better done outside the server in runFromConfig. I've verified the patch works on by Windows box. Thanks David Lao for the testing effort. Both patches, from Rakesh R and David Lao, hang on org.apache.zookeeper.server.ZxidRolloverTest for me. Flavio Junqueira , Thanks again for the reviews. In the latest patch I tried to fix the problem in ZooKeeperServerMain.java class. One thing I'm not able to understand is, why this fix is affecting ZxidRolloverTest testcases. Because am not seeing any relation ship between ZxidRolloverTest and transaction log closure in ZooKeeperServerMain . I hope you are looking at the latest patch which I attached on "Last Friday 17:244" For understanding the problem, it would be good if you can share logs, threaddumps etc. Is this consistently failiing and which test case in ZxidRolloverTest ?
        Hide
        Flavio Junqueira added a comment -

        why this fix is affecting ZxidRolloverTest testcases.

        I don´t know, I just know that the test hangs on Windows. I haven´t tested on Linux, but from the QA output I suppose it is not a problem.

        it would be good if you can share logs, threaddumps etc.

        Ok, I´ll upload when I have a chance.

        Show
        Flavio Junqueira added a comment - why this fix is affecting ZxidRolloverTest testcases. I don´t know, I just know that the test hangs on Windows. I haven´t tested on Linux, but from the QA output I suppose it is not a problem. it would be good if you can share logs, threaddumps etc. Ok, I´ll upload when I have a chance.
        Hide
        Rakesh R added a comment -

        ok. I tried in Windows 7 env but couldn't simulate the problem. Is this consistently failiing in your env, also would like to know which test case in ZxidRolloverTest ?

        Show
        Rakesh R added a comment - ok. I tried in Windows 7 env but couldn't simulate the problem. Is this consistently failiing in your env, also would like to know which test case in ZxidRolloverTest ?
        Hide
        Flavio Junqueira added a comment -

        Hi Rakesh, When you tried on Win 7, have you been able to get all tests to pass?

        Another thing, the patch does not apply cleanly to b3.4. I was thinking of including it in 3.4.6 if we can converge.

        Show
        Flavio Junqueira added a comment - Hi Rakesh, When you tried on Win 7, have you been able to get all tests to pass? Another thing, the patch does not apply cleanly to b3.4. I was thinking of including it in 3.4.6 if we can converge.
        Hide
        Rakesh R added a comment -

        Thanks Flavio for the interest. I've updated the branch3.4 patch. I'm having few problems in my Windows env and not able to run all the test cases. But the test cases which uses ZooKeeperServerMain class are running fine. I think, this fix wont affect other test cases, as its closing the FileTxnSnapLog stream in finally block. Any thoughts?

        Show
        Rakesh R added a comment - Thanks Flavio for the interest. I've updated the branch3.4 patch. I'm having few problems in my Windows env and not able to run all the test cases . But the test cases which uses ZooKeeperServerMain class are running fine. I think, this fix wont affect other test cases, as its closing the FileTxnSnapLog stream in finally block. Any thoughts?
        Hide
        Flavio Junqueira added a comment -

        It shouldn't be our goal to fix all test issues on windows here, so it is great if it already solves some. I'll have a look and check it in today, if I can't see any other issue.

        Show
        Flavio Junqueira added a comment - It shouldn't be our goal to fix all test issues on windows here, so it is great if it already solves some. I'll have a look and check it in today, if I can't see any other issue.
        Hide
        Rakesh R added a comment -

        It shouldn't be our goal to fix all test issues on windows here

        yeah, thats true and I agree on this.

        Show
        Rakesh R added a comment - It shouldn't be our goal to fix all test issues on windows here yeah, thats true and I agree on this.
        Hide
        Flavio Junqueira added a comment -

        I'm only going to suggest that we use tmpDir instead of rootDir to be consistent, but I'll make this change when committing. Otw, +1.

        If no one has a concern with this patch, I'll commit it soon.

        Show
        Flavio Junqueira added a comment - I'm only going to suggest that we use tmpDir instead of rootDir to be consistent, but I'll make this change when committing. Otw, +1. If no one has a concern with this patch, I'll commit it soon.
        Hide
        Raul Gutierrez Segales added a comment -

        One small nit:

        +            if (!f.delete())
        +                // double check for the file existence
        +                if (f.exists()) {
        +                    throw new IOException("Failed to delete file: " + f);
        +                }
        

        I believe all around ZK's source code if statements always have curly braces around their bodies, so:

        +            if (!f.delete()) {
        +                // double check for the file existence
        +                if (f.exists()) {
        +                    throw new IOException("Failed to delete file: " + f);
        +                }
        +            }
        

        would play nicer with the incumbent coding style.

        Show
        Raul Gutierrez Segales added a comment - One small nit: + if (!f.delete()) + // double check for the file existence + if (f.exists()) { + throw new IOException("Failed to delete file: " + f); + } I believe all around ZK's source code if statements always have curly braces around their bodies, so: + if (!f.delete()) { + // double check for the file existence + if (f.exists()) { + throw new IOException("Failed to delete file: " + f); + } + } would play nicer with the incumbent coding style.
        Hide
        Raul Gutierrez Segales added a comment -

        And one last nit, {} string extrapolation is nicer than concatenating strings. So instead of:

        +                    throw new IOException("Failed to delete file: " + f);
        

        maybe:

        +                    throw new IOException("Failed to delete file: {}", f);
        

        thanks.

        Show
        Raul Gutierrez Segales added a comment - And one last nit, {} string extrapolation is nicer than concatenating strings. So instead of: + throw new IOException("Failed to delete file: " + f); maybe: + throw new IOException("Failed to delete file: {}", f); thanks.
        Hide
        Flavio Junqueira added a comment -

        Thanks for the comments, Raul Gutierrez Segales. There is no need to generate a new patch, I'll make these changes before committing.

        Show
        Flavio Junqueira added a comment - Thanks for the comments, Raul Gutierrez Segales . There is no need to generate a new patch, I'll make these changes before committing.
        Hide
        Flavio Junqueira added a comment -

        Btw, the string extrapolation comment doesn't seem to be valid, it is not a log message.

        Show
        Flavio Junqueira added a comment - Btw, the string extrapolation comment doesn't seem to be valid, it is not a log message.
        Hide
        Rakesh R added a comment -

        Thank you for the comments. Yes, AFAIK lazy concatenation will be useful for efficient logging, where the string concatenation will only take place if the respective level is set/enabled. Here its throwing exception with err message and I also feel concatentation is fine.

        Show
        Rakesh R added a comment - Thank you for the comments. Yes, AFAIK lazy concatenation will be useful for efficient logging, where the string concatenation will only take place if the respective level is set/enabled. Here its throwing exception with err message and I also feel concatentation is fine.
        Hide
        Rakesh R added a comment -

        Thanks Flavio, Raul for the reviews. I just got some time and reworked the patch by using 'tmpDir'. Could you please have a look.

        Show
        Rakesh R added a comment - Thanks Flavio, Raul for the reviews. I just got some time and reworked the patch by using 'tmpDir'. Could you please have a look.
        Hide
        Hadoop QA added a comment -

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

        +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 (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/1816//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1816//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1816//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/12616986/ZOOKEEPER-1459.patch against trunk revision 1547702. +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 (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/1816//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1816//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1816//console This message is automatically generated.
        Hide
        Raul Gutierrez Segales added a comment -

        In:

        +        } finally {
        +            if (txnLog != null) {
        +                txnLog.close();
        +            }
        

        close can throw IOException. Is there anything we need to do about that?

        Show
        Raul Gutierrez Segales added a comment - In: + } finally { + if (txnLog != null) { + txnLog.close(); + } close can throw IOException. Is there anything we need to do about that?
        Hide
        Flavio Junqueira added a comment -

        We can catch the exception and log the error, I don't think there is anything else we need to do.

        Also, please try to concentrate comments in a single post, every new comment generates a new iteration and delays the issue being resolved.

        Rakesh R, if you don't have time, let me know and I can wrap this one up.

        Show
        Flavio Junqueira added a comment - We can catch the exception and log the error, I don't think there is anything else we need to do. Also, please try to concentrate comments in a single post, every new comment generates a new iteration and delays the issue being resolved. Rakesh R , if you don't have time, let me know and I can wrap this one up.
        Hide
        Rakesh R added a comment -

        Thanks again for the help.

        Do we need to have a separate exception block for 'txnLog.close();' ?
        I could see ZooKeeperServerMain#main(String[] args) is already catching 'Exception' and doing the necessary actions like, log the error and then exiting. In this case, mostly will throw IOException and this will fall into the invoker exception block. Whats your opinion.

        Show
        Rakesh R added a comment - Thanks again for the help. Do we need to have a separate exception block for 'txnLog.close();' ? I could see ZooKeeperServerMain#main(String[] args) is already catching 'Exception' and doing the necessary actions like, log the error and then exiting. In this case, mostly will throw IOException and this will fall into the invoker exception block. Whats your opinion.
        Hide
        Flavio Junqueira added a comment -

        Oops, you're right, runFromConfig already throws IOException, so I don't think we need to do anything else.

        Show
        Flavio Junqueira added a comment - Oops, you're right, runFromConfig already throws IOException, so I don't think we need to do anything else.
        Hide
        Flavio Junqueira added a comment -

        Also, are you planning on updating the 3.4 patch? Is it only the dir name rootDir -> tmpDir?

        Show
        Flavio Junqueira added a comment - Also, are you planning on updating the 3.4 patch? Is it only the dir name rootDir -> tmpDir?
        Hide
        Rakesh R added a comment -

        Flavio Junqueira Attached branch 3.4 patch. Yes, I've modified the rootDir -> tmpDir. Could you please have a look at it.

        Show
        Rakesh R added a comment - Flavio Junqueira Attached branch 3.4 patch. Yes, I've modified the rootDir -> tmpDir. Could you please have a look at it.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12617195/ZOOKEEPER-1459-branch-3_4.patch
        against trunk revision 1547702.

        +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-ZOOKEEPER-Build/1817//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/12617195/ZOOKEEPER-1459-branch-3_4.patch against trunk revision 1547702. +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-ZOOKEEPER-Build/1817//console This message is automatically generated.
        Hide
        Rakesh R added a comment -

        As the "ZOOKEEPER-1459-branch-3_4.patch" is prepared on branch 3.4, I feel this failure is expected. Please excuse.

        Show
        Rakesh R added a comment - As the " ZOOKEEPER-1459 -branch-3_4.patch" is prepared on branch 3.4, I feel this failure is expected. Please excuse.
        Hide
        Flavio Junqueira added a comment -

        Thanks, Rakesh R, it looks good.

        Show
        Flavio Junqueira added a comment - Thanks, Rakesh R , it looks good.
        Hide
        Flavio Junqueira added a comment -

        +1, thanks Rakesh R.

        Trunk: Committed revision 1548825.
        b3.4: Committed revision 1548826.

        Show
        Flavio Junqueira added a comment - +1, thanks Rakesh R . Trunk: Committed revision 1548825. b3.4: Committed revision 1548826.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in ZooKeeper-trunk #2144 (See https://builds.apache.org/job/ZooKeeper-trunk/2144/)
        ZOOKEEPER-1459. Standalone ZooKeeperServer is not closing
        the transaction log files on shutdown (Rakesh R via fpj) (fpj: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1548825)

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java
        • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java
        Show
        Hudson added a comment - FAILURE: Integrated in ZooKeeper-trunk #2144 (See https://builds.apache.org/job/ZooKeeper-trunk/2144/ ) ZOOKEEPER-1459 . Standalone ZooKeeperServer is not closing the transaction log files on shutdown (Rakesh R via fpj) (fpj: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1548825 ) /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.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:
            Rakesh R
            Reporter:
            Rakesh R
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development