ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1027

chroot not transparent in zoo_create()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 3.3.3
    • Fix Version/s: 3.4.0
    • Component/s: c client
    • Labels:
      None
    • Environment:

      Linux, ZooKeeper 3.3.3, C-client, java 1.6.0_17-b04, hotspot server vm

    • Hadoop Flags:
      Reviewed
    • Release Note:
      Correctly removes the chroot from the returned path in a call to zoo_create()
    • Tags:
      chroot zookeeper zoo_create

      Description

      I've recently started to use the chroot functionality (introduced in
      3.2.0) as part of my connect string.It mostly works as expected, but
      there is one case that is unexpected: when I create a path with
      zoo_create() I can retrieve the created path. This is very useful when
      you set the ZOO_SEQUENCE flag. Unfortunately the returned path
      includes the chroot as part of the path. This was unexpected to me: I
      expected that the chroot would be totally transparent. The
      documentation for zoo_create() says:
      "path_buffer : Buffer which will be filled with the path of the new
      node (this might be different than the supplied path because of the
      ZOO_SEQUENCE flag)."

      This gave me the impression that this flag is the only reason the
      returned path is different from the created path, but apparently it's
      not. Is this a bug or intended behavior?
      I workaround this issue now by remembering the chroot in
      my wrapper code and after a call to zoo_create() i check if the returned
      path starts with the chroot. If it does, I remove it.

      My use case is to create a path with a sequence number and then delete
      this path later. Unfortunately I cannot delete the path because it has
      the chroot prepended to it, and thus it will result in two chroots.

      I believe this only affects the create functions.

      1. ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
        2 kB
        Thijs Terlouw
      2. ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
        2 kB
        Mahadev konar
      3. ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
        2 kB
        Mahadev konar
      4. ZOOKEEPER-1027-PATCH-TRUNK3.patch
        3 kB
        Thijs Terlouw
      5. ZOOKEEPER-1027-PATCH-TRUNK2.patch
        3 kB
        Thijs Terlouw

        Issue Links

          Activity

          Hide
          Thijs Terlouw added a comment -

          I realized that I probably need to test it again using the new 3.3.3 C-client code. I only tested against the 3.3.3 server

          Show
          Thijs Terlouw added a comment - I realized that I probably need to test it again using the new 3.3.3 C-client code. I only tested against the 3.3.3 server
          Hide
          Thijs Terlouw added a comment -

          tested with 3.3.3 C-client, same problem

          Show
          Thijs Terlouw added a comment - tested with 3.3.3 C-client, same problem
          Hide
          Thijs Terlouw added a comment -

          Two problems afaik:

          (1) I noticed a sub_string(zh, server_path) function that contains a bug at line 840 in zookeeper.c:

          if (strncmp(server_path, zh->chroot, strlen(zh->chroot) != 0)) {

          notice that "Unable to render embedded object: File (= 0" appears inside the 3rd argument to the strncmp() function) not found. So it executes

          if(strncmp(server_path, zh->chroot, 1)) {

          which will always fail if the first character is the same. The next if() also fails (server_path length is not equal to chroot usually). This if() has the potential to cause wrong results btw. Then finally it returns the string with the chroot stripped off. So it will strip off the chroot, but for the wrong reason.

          (2) the sub_string() function is only called for do_foreach_watcher() function (zk_hashtable.c line 273). It doesn't seem to be called in the zoo_create/zoo_acreate/add_string_completion path. This is the bug that influences this bug report.

          Show
          Thijs Terlouw added a comment - Two problems afaik: (1) I noticed a sub_string(zh, server_path) function that contains a bug at line 840 in zookeeper.c: if (strncmp(server_path, zh->chroot, strlen(zh->chroot) != 0)) { notice that " Unable to render embedded object: File (= 0" appears inside the 3rd argument to the strncmp() function) not found. So it executes if(strncmp(server_path, zh->chroot, 1)) { which will always fail if the first character is the same. The next if() also fails (server_path length is not equal to chroot usually). This if() has the potential to cause wrong results btw. Then finally it returns the string with the chroot stripped off. So it will strip off the chroot, but for the wrong reason. (2) the sub_string() function is only called for do_foreach_watcher() function (zk_hashtable.c line 273). It doesn't seem to be called in the zoo_create/zoo_acreate/add_string_completion path. This is the bug that influences this bug report.
          Hide
          Thijs Terlouw added a comment -

          (1) fix:

          // ZOOKEEPER-1027/1 : fix strncmp + do not return NULL
          if (strncmp(server_path, zh->chroot, strlen(zh->chroot)) != 0)

          Unknown macro: { LOG_ERROR(("server path %s does not include chroot path %s", server_path, zh->chroot)); return (char *) server_path; }
          Show
          Thijs Terlouw added a comment - (1) fix: // ZOOKEEPER-1027 /1 : fix strncmp + do not return NULL if (strncmp(server_path, zh->chroot, strlen(zh->chroot)) != 0) Unknown macro: { LOG_ERROR(("server path %s does not include chroot path %s", server_path, zh->chroot)); return (char *) server_path; }
          Hide
          Thijs Terlouw added a comment -
           
          diff -Naur zookeeper-3.3.3/src/c/src/zookeeper.c zookeeper-3.3.3-patched/src/c/src/zookeeper.c
          --- zookeeper-3.3.3/src/c/src/zookeeper.c   2011-02-24 07:44:56.000000000 +0800
          +++ zookeeper-3.3.3-patched/src/c/src/zookeeper.c   2011-03-25 11:11:23.000000000 +0800
          @@ -837,10 +837,11 @@
               char *ret_str;
               if (zh->chroot == NULL)
                   return (char *) server_path;
          -    if (strncmp(server_path, zh->chroot, strlen(zh->chroot) != 0)) {
          +    //ZOOKEEPER-1027/1 : do not return NULL and fix strncmp
          +    if (strncmp(server_path, zh->chroot, strlen(zh->chroot)) != 0) {
                   LOG_ERROR(("server path %s does not include chroot path %s",
                              server_path, zh->chroot));
          -        return NULL;
          +        return (char *) server_path;
               }
               if (strlen(server_path) == strlen(zh->chroot)) {
                   //return "/"
          
          Show
          Thijs Terlouw added a comment - diff -Naur zookeeper-3.3.3/src/c/src/zookeeper.c zookeeper-3.3.3-patched/src/c/src/zookeeper.c --- zookeeper-3.3.3/src/c/src/zookeeper.c 2011-02-24 07:44:56.000000000 +0800 +++ zookeeper-3.3.3-patched/src/c/src/zookeeper.c 2011-03-25 11:11:23.000000000 +0800 @@ -837,10 +837,11 @@ char *ret_str; if (zh->chroot == NULL) return (char *) server_path; - if (strncmp(server_path, zh->chroot, strlen(zh->chroot) != 0)) { + //ZOOKEEPER-1027/1 : do not return NULL and fix strncmp + if (strncmp(server_path, zh->chroot, strlen(zh->chroot)) != 0) { LOG_ERROR(("server path %s does not include chroot path %s", server_path, zh->chroot)); - return NULL; + return (char *) server_path; } if (strlen(server_path) == strlen(zh->chroot)) { //return "/"
          Hide
          Thijs Terlouw added a comment -

          The call to sub_string() appears to be needed inside zookeeper_process() near line 2062 in zookeeper.c
          I'll submit a full patch later

          Show
          Thijs Terlouw added a comment - The call to sub_string() appears to be needed inside zookeeper_process() near line 2062 in zookeeper.c I'll submit a full patch later
          Hide
          Thijs Terlouw added a comment -

          ZOOKEEPER-1027 patch by Thijs Terlouw

          Show
          Thijs Terlouw added a comment - ZOOKEEPER-1027 patch by Thijs Terlouw
          Hide
          Thijs Terlouw added a comment -

          I've tried to make a failing unit test, but strangely enough the unit tests return the correct result (even without my patch). I still have to investigate this further, but I believe the two bugs are correctly solved.

          Show
          Thijs Terlouw added a comment - I've tried to make a failing unit test, but strangely enough the unit tests return the correct result (even without my patch). I still have to investigate this further, but I believe the two bugs are correctly solved.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12474602/zookeeper-3.3.3.patch
          against trunk revision 1082362.

          +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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/200//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/12474602/zookeeper-3.3.3.patch against trunk revision 1082362. +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://hudson.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/200//console This message is automatically generated.
          Hide
          Thijs Terlouw added a comment -

          I can confirm that this patch solves the problem I was seeing. I have tried a lot of things to make the unit tests fail for testChroot() but somehow the unit test always succeeds. Perhaps some compilation options are different or different concurrency? Just guessing, it's very strange that the unit tests don't fail

          Show
          Thijs Terlouw added a comment - I can confirm that this patch solves the problem I was seeing. I have tried a lot of things to make the unit tests fail for testChroot() but somehow the unit test always succeeds. Perhaps some compilation options are different or different concurrency? Just guessing, it's very strange that the unit tests don't fail
          Hide
          Andrei Savu added a comment -

          I've seen a similar issue affecting the zookeeper python client (see ZOOKEEPER-995) and I've attached a failing unit test.

          Show
          Andrei Savu added a comment - I've seen a similar issue affecting the zookeeper python client (see ZOOKEEPER-995 ) and I've attached a failing unit test.
          Hide
          Patrick Hunt added a comment -

          Thijs, thanks for this - would you mind updating this patch for:

          1) get it to patch successfully against trunk (you'll probably need two patch files, one for 3.3 branch and one for trunk)
          2) add a C based test based on Andrei's example from python (possible?)
          3) add the ZOOKEEPER-995 changes to this patch

          Show
          Patrick Hunt added a comment - Thijs, thanks for this - would you mind updating this patch for: 1) get it to patch successfully against trunk (you'll probably need two patch files, one for 3.3 branch and one for trunk) 2) add a C based test based on Andrei's example from python (possible?) 3) add the ZOOKEEPER-995 changes to this patch
          Hide
          Patrick Hunt added a comment -

          cancelling patch for now while we address the comments (tests primarily)

          Show
          Patrick Hunt added a comment - cancelling patch for now while we address the comments (tests primarily)
          Hide
          Thijs Terlouw added a comment -

          Hi Patrick, I will try to create an improved patch next weekend, currently I have no time.

          Show
          Thijs Terlouw added a comment - Hi Patrick, I will try to create an improved patch next weekend, currently I have no time.
          Hide
          Thijs Terlouw added a comment -

          New patch attached which includes unit tests. Should apply cleanly to the trunk, but I have no idea how to trigger the Hadoop QA Bot...

          Show
          Thijs Terlouw added a comment - New patch attached which includes unit tests. Should apply cleanly to the trunk, but I have no idea how to trigger the Hadoop QA Bot...
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12477090/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
          against trunk revision 1091841.

          +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 appears to introduce 1 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/hudson/job/PreCommit-ZOOKEEPER-Build/229//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/229//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/229//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/12477090/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch against trunk revision 1091841. +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 appears to introduce 1 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/hudson/job/PreCommit-ZOOKEEPER-Build/229//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/229//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/229//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          +1 the patch looks good to me.

          Show
          Mahadev konar added a comment - +1 the patch looks good to me.
          Hide
          Mahadev konar added a comment -

          one minor nit:

           LOG_ERROR(("server path %s does not include chroot path %s",
                             server_path, zh->chroot));
          

          I think we should comment this out as well, since this wont be an error on a create call. What do you think Thijs?

          Show
          Mahadev konar added a comment - one minor nit: LOG_ERROR(("server path %s does not include chroot path %s", server_path, zh->chroot)); I think we should comment this out as well, since this wont be an error on a create call. What do you think Thijs?
          Hide
          Mahadev konar added a comment -

          marking it for just 3.4 since 3.3.3 isnt planned yet.

          Show
          Mahadev konar added a comment - marking it for just 3.4 since 3.3.3 isnt planned yet.
          Hide
          Thijs Terlouw added a comment -

          I think just for 3.4 is good, because it changes the behavior of the C-client a little bit and that should probably not happen in a small patch release such as 3.3.3

          About the LOG_ERROR() message: I didn't look in detail at the code-path for creating, so I didn't consider this case. It's a non-functional change, so removing it is no problem for me

          Show
          Thijs Terlouw added a comment - I think just for 3.4 is good, because it changes the behavior of the C-client a little bit and that should probably not happen in a small patch release such as 3.3.3 About the LOG_ERROR() message: I didn't look in detail at the code-path for creating, so I didn't consider this case. It's a non-functional change, so removing it is no problem for me
          Hide
          Benjamin Reed added a comment -

          what are we waiting on for this patch? is is simply to remove the LOG_ERROR?

          i'm also wondering if we shouldn't still return NULL if there is a suffix error in sub_string. it seems like we should keep returning a NULL.

          Show
          Benjamin Reed added a comment - what are we waiting on for this patch? is is simply to remove the LOG_ERROR? i'm also wondering if we shouldn't still return NULL if there is a suffix error in sub_string. it seems like we should keep returning a NULL.
          Hide
          Thijs Terlouw added a comment -

          @benjamin:

          • I was under the impression that Mahadev indeed wanted to remove the LOG_ERROR, but that this patch would be incorporated into 3.4.0 due to slight change in behavior (some people might rely on the fact that now the chroot is included)
            I added the LOG_ERROR() because chroot should be NULL if it's not used, and otherwise it should always be included in the server path, but perhaps I overlooked something here.
          • I think NULL should not be returned from sub_string, for the following reasons:
          • in the current code NULL is never returned due to a parenthesis bug on the enclosing if() statement. strlen(zh->chroot) != 0 will always return 1 and the strncmp() function will thus compare 1 character of server_path and zh->chroot. Since they both start with "/" strncmp will equal (0) and thus the if(0) always fails.
          • other cases in sub_string all return the current server_path as default value
          • the documentation (in zookeeper.h) only states that the path is NULL when the type is ZOO_SESSION_EVENT
          • the function documentation for sub_string() says it should return the exact path if chroot is not included

          as far as I am concerned, this patch (or without LOG_ERROR) can just be incorporated in the next release, but do let mahadev have another look at it

          Show
          Thijs Terlouw added a comment - @benjamin: I was under the impression that Mahadev indeed wanted to remove the LOG_ERROR, but that this patch would be incorporated into 3.4.0 due to slight change in behavior (some people might rely on the fact that now the chroot is included) I added the LOG_ERROR() because chroot should be NULL if it's not used, and otherwise it should always be included in the server path, but perhaps I overlooked something here. I think NULL should not be returned from sub_string, for the following reasons: in the current code NULL is never returned due to a parenthesis bug on the enclosing if() statement. strlen(zh->chroot) != 0 will always return 1 and the strncmp() function will thus compare 1 character of server_path and zh->chroot. Since they both start with "/" strncmp will equal (0) and thus the if(0) always fails. other cases in sub_string all return the current server_path as default value the documentation (in zookeeper.h) only states that the path is NULL when the type is ZOO_SESSION_EVENT the function documentation for sub_string() says it should return the exact path if chroot is not included as far as I am concerned, this patch (or without LOG_ERROR) can just be incorporated in the next release, but do let mahadev have another look at it
          Hide
          Mahadev konar added a comment -

          Retrying hudson.

          Show
          Mahadev konar added a comment - Retrying hudson.
          Hide
          Mahadev konar added a comment -

          retrying Thijs patch.

          Show
          Mahadev konar added a comment - retrying Thijs 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/12483913/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
          against trunk revision 1140017.

          +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/354//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/354//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/354//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/12483913/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch against trunk revision 1140017. +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/354//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/354//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/354//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          The patch has gone stale due to Multi checkin.

          Show
          Mahadev konar added a comment - The patch has gone stale due to Multi checkin.
          Hide
          Mahadev konar added a comment -

          Updated patch to apply on trunk. Thijis can you please take a look and see in case I missed something?

          I didnt get a chance to run the tests since I currently dont have access to a linux box.

          Show
          Mahadev konar added a comment - Updated patch to apply on trunk. Thijis can you please take a look and see in case I missed something? I didnt get a chance to run the tests since I currently dont have access to a linux box.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485539/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
          against trunk revision 1142377.

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/373//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/373//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/373//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/12485539/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch against trunk revision 1142377. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/373//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/373//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/373//console This message is automatically generated.
          Hide
          Thijs Terlouw added a comment -

          Patch looks fine, i just compared yours and mine. There are some open issues from the discussion of Benjamin and me, if you agree with me, then the patch can be released like this

          Show
          Thijs Terlouw added a comment - Patch looks fine, i just compared yours and mine. There are some open issues from the discussion of Benjamin and me, if you agree with me, then the patch can be released like this
          Hide
          Thijs Terlouw added a comment -

          I see that Hadoop QA didn't agree:

          -1 core tests. The patch failed core unit tests.

          but there are no failed tests in the test report...

          Show
          Thijs Terlouw added a comment - I see that Hadoop QA didn't agree: -1 core tests. The patch failed core unit tests. but there are no failed tests in the test report...
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485539/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch
          against trunk revision 1144087.

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384//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/12485539/ZOOKEEPER-1027-TRUNK_WITH_TESTS.patch against trunk revision 1144087. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          Looks like some issue in compilation. Thijs can you please take a look? It might take you less time than me to create a patch for this one. Let me know if you cant.

          Here's the issue:

          https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384/console

          At the bottom of the logs there is a compilation error.

          Show
          Mahadev konar added a comment - Looks like some issue in compilation. Thijs can you please take a look? It might take you less time than me to create a patch for this one. Let me know if you cant. Here's the issue: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/384/console At the bottom of the logs there is a compilation error.
          Hide
          Thijs Terlouw added a comment -

          It seems the code has been refactored, a large block of code has been moved from the function
          zookeeper_process(zhandle_t *zh, int events)
          to
          process_sync_completion(cptr, sc, ia);

          the patch requires access to the zhandle_t pointer though, so you get compilation issues about missing "zh"

          The simplest solution is to add a 4th parameter (zhandle_t *zh) to the process_sync_completion() function. It compiles and runs the unit tests correctly after this change, but it's a bit complicated to generate a new patch for me, because I am not sure which version to patch against.

          Show
          Thijs Terlouw added a comment - It seems the code has been refactored, a large block of code has been moved from the function zookeeper_process(zhandle_t *zh, int events) to process_sync_completion(cptr, sc, ia); the patch requires access to the zhandle_t pointer though, so you get compilation issues about missing "zh" The simplest solution is to add a 4th parameter (zhandle_t *zh) to the process_sync_completion() function. It compiles and runs the unit tests correctly after this change, but it's a bit complicated to generate a new patch for me, because I am not sure which version to patch against.
          Hide
          Mahadev konar added a comment -

          Thijs,
          The version to patch against is trunk. Would you be able to provide a patch in that case?

          Show
          Mahadev konar added a comment - Thijs, The version to patch against is trunk. Would you be able to provide a patch in that case?
          Hide
          Thijs Terlouw added a comment -

          no problem, I will do it later today!

          Show
          Thijs Terlouw added a comment - no problem, I will do it later today!
          Hide
          Thijs Terlouw added a comment -

          ZK-1027 patch updated for trunk. Adds a 4th parameter zhandle_t* to process_sync_completion()

          Show
          Thijs Terlouw added a comment - ZK-1027 patch updated for trunk. Adds a 4th parameter zhandle_t* to process_sync_completion()
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12486152/ZOOKEEPER-1027-PATCH-TRUNK2.patch
          against trunk revision 1146025.

          +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/392//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/12486152/ZOOKEEPER-1027-PATCH-TRUNK2.patch against trunk revision 1146025. +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/392//console This message is automatically generated.
          Hide
          Thijs Terlouw added a comment -

          previous patch had some issues with directory name, fixed that and added a new patch

          Show
          Thijs Terlouw added a comment - previous patch had some issues with directory name, fixed that and added a new 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/12486412/ZOOKEEPER-1027-PATCH-TRUNK3.patch
          against trunk revision 1146025.

          +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/395//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/395//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/395//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/12486412/ZOOKEEPER-1027-PATCH-TRUNK3.patch against trunk revision 1146025. +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/395//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/395//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/395//console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          I just pushed this. Thanks Thijs.

          Show
          Mahadev konar added a comment - I just pushed this. Thanks Thijs.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1255 (See https://builds.apache.org/job/ZooKeeper-trunk/1255/)
          ZOOKEEPER-1027. chroot not transparent in zoo_create() (Thijs Terlouw via mahadev)

          mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1150802
          Files :

          • /zookeeper/trunk/src/c/src/zookeeper.c
          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/c/tests/TestClient.cc
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1255 (See https://builds.apache.org/job/ZooKeeper-trunk/1255/ ) ZOOKEEPER-1027 . chroot not transparent in zoo_create() (Thijs Terlouw via mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1150802 Files : /zookeeper/trunk/src/c/src/zookeeper.c /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/c/tests/TestClient.cc
          Hide
          Dheeraj Agrawal added a comment -

          Mahadev, this patch breaks windows build.
          //ZOOKEEPER-1027
          const char* client_path = sub_string(zh, res.path);

          Windows doesn't like declaration of variables in middle of the code... The declaration of const char* client_path should be made just after the if condition at line 1898.
          1898 if (sc->rc==0) {
          1899 const char* client_path;

          Show
          Dheeraj Agrawal added a comment - Mahadev, this patch breaks windows build. // ZOOKEEPER-1027 const char* client_path = sub_string(zh, res.path); Windows doesn't like declaration of variables in middle of the code... The declaration of const char* client_path should be made just after the if condition at line 1898. 1898 if (sc->rc==0) { 1899 const char* client_path;
          Hide
          Mahadev konar added a comment -

          dheeraj,
          Can you please open a new jira and file a patch for it? Please mark it as a blocker for 3.4.0.

          thanks

          Show
          Mahadev konar added a comment - dheeraj, Can you please open a new jira and file a patch for it? Please mark it as a blocker for 3.4.0. thanks
          Hide
          Dheeraj Agrawal added a comment -

          yes, i have created a sub-task for this jira. will be uploading patch in some time.

          Show
          Dheeraj Agrawal added a comment - yes, i have created a sub-task for this jira. will be uploading patch in some time.
          Hide
          Mahadev konar added a comment -

          thanks a lot Dheeraj!

          Show
          Mahadev konar added a comment - thanks a lot Dheeraj!

            People

            • Assignee:
              Thijs Terlouw
              Reporter:
              Thijs Terlouw
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development