ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-622

Test for pending watches in send_set_watches should be moved

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.0
    • Component/s: c client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Valgrind found:

      ==2357== Conditional jump or move depends on uninitialised value(s)
      ==2357== at 0x807FDCA: check_events (zookeeper.c:1180)
      ==2357== by 0x808043A: zookeeper_process (zookeeper.c:1775)
      ==2357== by 0x806A21B: Zookeeper_close::testCloseConnected1() (TestZookeeperClose.cc:161)
      ==2357== by 0x806C6BF: CppUnit::TestCaller<Zookeeper_close>::runTest() (TestCaller.h:166)

      zookeeper.c:1180 was the first if in send_set_watches.

      1. ZOOKEEPER-622.patch
        1 kB
        Benjamin Reed
      2. ZOOKEEPER-622.patch
        22 kB
        Steven Cheng
      3. ZOOKEEPER-622.patch
        20 kB
        Steven Cheng
      4. ZOOKEEPER-622.patch
        1 kB
        Steven Cheng

        Issue Links

          Activity

          Hide
          Steven Cheng added a comment -

          This patch moves the check for outstanding watches in send_set_watches, going to try to figure out how to test this later, any suggestions?

          Show
          Steven Cheng added a comment - This patch moves the check for outstanding watches in send_set_watches, going to try to figure out how to test this later, any suggestions?
          Hide
          Mahadev konar added a comment -

          this looks like a critical problem. This could cause setwatches never to be set if the count is initialized to 0 for all the three watches. We should definitely have a test case for this. How about setting a watch on a zookeeper server and shutting it down and making sure that the watch gets fired on the second server it reconnects to?

          Show
          Mahadev konar added a comment - this looks like a critical problem. This could cause setwatches never to be set if the count is initialized to 0 for all the three watches. We should definitely have a test case for this. How about setting a watch on a zookeeper server and shutting it down and making sure that the watch gets fired on the second server it reconnects to?
          Hide
          Steven Cheng added a comment -

          Sounds good Mahadev, I'll get started on it.

          Show
          Steven Cheng added a comment - Sounds good Mahadev, I'll get started on it.
          Hide
          Mahadev konar added a comment -

          steve, any update on this? we should try to get this into 3.3.0

          Show
          Mahadev konar added a comment - steve, any update on this? we should try to get this into 3.3.0
          Hide
          Steven Cheng added a comment -

          Looks like I need to add multi server handling to the C tests to manage this, I'll try to get a rough cut done and post the patch.

          Show
          Steven Cheng added a comment - Looks like I need to add multi server handling to the C tests to manage this, I'll try to get a rough cut done and post the patch.
          Hide
          Steven Cheng added a comment -

          Started on adding multi server testing for C client, started creating the scripts, got stuck. main() in TestDriver.cc will always start the single server.

          This patch has the scripts that I created so far.

          It looks like adding this will need quite a few changes. Should I open a new JIRA?

          Show
          Steven Cheng added a comment - Started on adding multi server testing for C client, started creating the scripts, got stuck. main() in TestDriver.cc will always start the single server. This patch has the scripts that I created so far. It looks like adding this will need quite a few changes. Should I open a new JIRA?
          Hide
          Patrick Hunt added a comment -

          Ben, can you look at this as well? seems related to the close?

          Show
          Patrick Hunt added a comment - Ben, can you look at this as well? seems related to the 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/12430524/ZOOKEEPER-622.patch
          against trunk revision 903483.

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

          +1 tests included. The patch appears to include 36 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 generated 6 release audit warnings (more than the trunk's current 1 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-h7.grid.sp2.yahoo.net/60/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/60/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/60/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/60/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/12430524/ZOOKEEPER-622.patch against trunk revision 903483. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 36 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 generated 6 release audit warnings (more than the trunk's current 1 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-h7.grid.sp2.yahoo.net/60/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/60/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/60/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/60/console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          good catch steven and excellent patch! will you put the license header in the generateCfgs.sh script? will you also put the header at the end of localserverlist (making it a footer to avoid a release warning?

          Show
          Benjamin Reed added a comment - good catch steven and excellent patch! will you put the license header in the generateCfgs.sh script? will you also put the header at the end of localserverlist (making it a footer to avoid a release warning?
          Hide
          Benjamin Reed added a comment -

          sorry, i looked at this a bit more closely and i realized that we can't really assume that there is a /tmp. we should be using $

          {base_dir}

          /build/tmp/ i'll take a whack at fixing the zkMultiServer.sh.

          Show
          Benjamin Reed added a comment - sorry, i looked at this a bit more closely and i realized that we can't really assume that there is a /tmp. we should be using $ {base_dir} /build/tmp/ i'll take a whack at fixing the zkMultiServer.sh.
          Hide
          Benjamin Reed added a comment -

          hey steven, it appears the test case isn't testing anything. can you clarify how it works?

          Show
          Benjamin Reed added a comment - hey steven, it appears the test case isn't testing anything. can you clarify how it works?
          Hide
          Steven Cheng added a comment -

          Hi Ben, sorry about that, this was just a rough cut patch, I got stuck when I tried to add zkMultiServer.sh into the tests since the Zookeeper server gets started in main() rather than in the test classes, so I posted it here to see if I was on the right track. Not sure if that was the right thing to do, mainly didn't want to go through and start changing all of the test setup without checking first.

          If there isn't anything wrong with moving the Zookeeper server startup into the setup() methods of the test classes, I can probably get the patch starting up multiple servers in /tmp and the basic test in place after some work. To handle $base_dir I was planning to generate the config files somewhere in $base_dir and point the server to them.

          Show
          Steven Cheng added a comment - Hi Ben, sorry about that, this was just a rough cut patch, I got stuck when I tried to add zkMultiServer.sh into the tests since the Zookeeper server gets started in main() rather than in the test classes, so I posted it here to see if I was on the right track. Not sure if that was the right thing to do, mainly didn't want to go through and start changing all of the test setup without checking first. If there isn't anything wrong with moving the Zookeeper server startup into the setup() methods of the test classes, I can probably get the patch starting up multiple servers in /tmp and the basic test in place after some work. To handle $base_dir I was planning to generate the config files somewhere in $base_dir and point the server to them.
          Hide
          Benjamin Reed added a comment -

          sorry steven, i didn't notice that you had commented. yes, please finish the test you can make simplifying assumptions such as /tmp and i can help you clean it up once things are working. thanx!

          Show
          Benjamin Reed added a comment - sorry steven, i didn't notice that you had commented. yes, please finish the test you can make simplifying assumptions such as /tmp and i can help you clean it up once things are working. thanx!
          Hide
          Steven Cheng added a comment -

          A working test case for watch transfer with multiple hosts. I left testScript in there because it is quite useful for basic sanity testing for the scripts.

          Show
          Steven Cheng added a comment - A working test case for watch transfer with multiple hosts. I left testScript in there because it is quite useful for basic sanity testing for the scripts.
          Hide
          Steven Cheng added a comment -

          This patch works but is still in rough shape! The multi host test servers are started on separate ports and are running concurrently with the single host test server. Would be helpful to make this more clear.

          Paths still hardcoded to /tmp.

          zooX.cfg are in the patch but are generated by generateCfgs.sh

          I encountered a ZCONNECTIONLOSS from an operation executed after the client reconnects to a new host. Adds an awkward section to the test case. I guess in some sense the client should not be oblivious to one of these "lucky" disconnect/reconnect situations.

          Show
          Steven Cheng added a comment - This patch works but is still in rough shape! The multi host test servers are started on separate ports and are running concurrently with the single host test server. Would be helpful to make this more clear. Paths still hardcoded to /tmp. zooX.cfg are in the patch but are generated by generateCfgs.sh I encountered a ZCONNECTIONLOSS from an operation executed after the client reconnects to a new host. Adds an awkward section to the test case. I guess in some sense the client should not be oblivious to one of these "lucky" disconnect/reconnect situations.
          Hide
          Benjamin Reed added a comment -

          this is the patch with just the bug fix in it. the system test has been moved to the related jira.

          Show
          Benjamin Reed added a comment - this is the patch with just the bug fix in it. the system test has been moved to the related jira.
          Hide
          Hadoop QA added a comment -

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

          +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 tests are needed for 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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/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/12437783/ZOOKEEPER-622.patch against trunk revision 918757. +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 tests are needed for 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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/77/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

          +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 tests are needed for 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 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-h8.grid.sp2.yahoo.net/132/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/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/12437783/ZOOKEEPER-622.patch against trunk revision 919640. +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 tests are needed for 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 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-h8.grid.sp2.yahoo.net/132/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/132/console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          I just committed this. thanks ben and steven.

          Show
          Mahadev konar added a comment - I just committed this. thanks ben and steven.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #716 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/716/)
          . Test for pending watches in send_set_watches should be moved (ben and steven via mahadev)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #716 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/716/ ) . Test for pending watches in send_set_watches should be moved (ben and steven via mahadev)

            People

            • Assignee:
              Benjamin Reed
              Reporter:
              Steven Cheng
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development