ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-78

added a high level protocol/feature - for easy Leader Election or exclusive Write Lock creation

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0
    • Fix Version/s: 3.2.0
    • Component/s: java client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Here's a patch which adds a little WriteLock helper class for performing leader elections or creating exclusive locks in some directory znode. Note its an early cut; am sure we can improve it over time. The aim is to avoid folks having to use the low level ZK stuff but provide a simpler high level abstraction.

      1. ZOOKEEPER-78.patch
        36 kB
        Mahadev konar
      2. ZOOKEEPER-78.patch
        34 kB
        Mahadev konar
      3. ZOOKEEPER-78.patch
        49 kB
        Mahadev konar
      4. ZOOKEEPER-78.patch
        172 kB
        Mahadev konar
      5. ZOOKEEPER-78.patch
        173 kB
        Mahadev konar
      6. ZOOKEEPER-78.patch
        174 kB
        Mahadev konar
      7. ZOOKEEPER-78.patch
        176 kB
        Mahadev konar
      8. ZOOKEEPER-78.patch
        179 kB
        Mahadev konar
      9. ZOOKEEPER-78.patch
        181 kB
        Mahadev konar
      10. ZOOKEEPER-78.patch
        183 kB
        Mahadev konar
      11. using_zookeeper_facade.patch
        46 kB
        james strachan
      12. patch_with_including_Benjamin's_fix.patch
        37 kB
        james strachan

        Issue Links

          Activity

          Hide
          Mahadev konar added a comment -

          assiging this to james since he is working on this.

          Show
          Mahadev konar added a comment - assiging this to james since he is working on this.
          Hide
          Patrick Hunt added a comment -

          We should document the leader election on the recipes page first.

          Show
          Patrick Hunt added a comment - We should document the leader election on the recipes page first.
          Hide
          Flavio Junqueira added a comment -

          This is a nice implementation, James. Good job! My two comments are:

          1- It might be a good idea to throw the exceptions instead of trying to catch them and retry. You will end up with a cleaner code;
          2- I'm not sure if it is necessary to add the log4j configuration. Is there a particular reason for including it or it is there by accident?

          Show
          Flavio Junqueira added a comment - This is a nice implementation, James. Good job! My two comments are: 1- It might be a good idea to throw the exceptions instead of trying to catch them and retry. You will end up with a cleaner code; 2- I'm not sure if it is necessary to add the log4j configuration. Is there a particular reason for including it or it is there by accident?
          Hide
          james strachan added a comment -

          Thanks Flavio!

          Totally agreed on 1. Strictly speaking we should catch all exceptions and handle them properly (which may mean throwing some, or responding properly to others or whatever).

          One of the main reasons for the retry logic was to avoid errors like trying to create a znode that already exists or loosing connection to the ZK server etc - but we should go through all possible exceptions and handle them much cleaner.

          In particular we really need test cases that show the server closing and restarting during the process of acquiring the lock or after a lock owner has the lock etc.

          I figured I'd send a patch first and see if anyone else had a better implementation lying around - or knew a neater way to solve this - before I spent too much time getting it totally correct etc.

          For 2) I just added that so that when running the unit tests you could see INFO or DEBUG level logging etc (particularly when running in your IDE)

          Show
          james strachan added a comment - Thanks Flavio! Totally agreed on 1. Strictly speaking we should catch all exceptions and handle them properly (which may mean throwing some, or responding properly to others or whatever). One of the main reasons for the retry logic was to avoid errors like trying to create a znode that already exists or loosing connection to the ZK server etc - but we should go through all possible exceptions and handle them much cleaner. In particular we really need test cases that show the server closing and restarting during the process of acquiring the lock or after a lock owner has the lock etc. I figured I'd send a patch first and see if anyone else had a better implementation lying around - or knew a neater way to solve this - before I spent too much time getting it totally correct etc. For 2) I just added that so that when running the unit tests you could see INFO or DEBUG level logging etc (particularly when running in your IDE)
          Hide
          james strachan added a comment -

          here's an updated patch which has better documentation and includes the recipe documentation linked to from the javadoc - but which could be used stand alone as well if required.

          I've also included the description from ZOOKEEPER-79 as well

          Show
          james strachan added a comment - here's an updated patch which has better documentation and includes the recipe documentation linked to from the javadoc - but which could be used stand alone as well if required. I've also included the description from ZOOKEEPER-79 as well
          Hide
          james strachan added a comment -

          Here is an improved version.

          • we use more optimal comparison by using a ZNodeName object which caches the prefix & sequence number for ordering node names. We can also use this to order node names using different prefixes - maybe useful for read/write locks
          • fixed a bug and enhanced the test case so that we now test that a leader is established; then when that leader fails another leader/owner is created
          Show
          james strachan added a comment - Here is an improved version. we use more optimal comparison by using a ZNodeName object which caches the prefix & sequence number for ordering node names. We can also use this to order node names using different prefixes - maybe useful for read/write locks fixed a bug and enhanced the test case so that we now test that a leader is established; then when that leader fails another leader/owner is created
          Hide
          james strachan added a comment -

          BTW I just deleted the other 2 patches to avoid confusion; the latest patch includes the previous changes etc

          Show
          james strachan added a comment - BTW I just deleted the other 2 patches to avoid confusion; the latest patch includes the previous changes etc
          Hide
          Benjamin Reed added a comment -

          There is a bug in this block of code:

                 while (!closed.get() && id == null) {
                     retryDelay(attempt++);
                     try {
                         id = zookeeper.create(dir + "/x-", data, acl, EPHEMERAL | SEQUENCE);
                         idName = new ZNodeName(id);
                         if (LOG.isDebugEnabled()) {
                             LOG.debug("Created id: " + id);
                         }
                     } catch (KeeperException e) {
                         LOG.warn("Caught: " + e, e);
                     } catch (InterruptedException e) {
                         LOG.warn("Caught: " + e, e);
                     }
                 }
          

          zookeeper.create is not idempotent, so blindly retrying will land you into problems:

          1) You start the create, a connection error happens, the create completes but you don't get a response
          2) You retry your create
          3) You may end up waiting on the znode from step 1) which will not go away

          I've been thinking of easy ways of getting around this problem and the easiest seems to be constructing the name as x-identifier-sequenceNumber. You could use the hostname or the sessionid as the identifier. When you do the retry you have to do a getChildren() to see if there are any znodes with your identifier and then use that znode if it exists.

          Show
          Benjamin Reed added a comment - There is a bug in this block of code: while (!closed.get() && id == null ) { retryDelay(attempt++); try { id = zookeeper.create(dir + "/x-" , data, acl, EPHEMERAL | SEQUENCE); idName = new ZNodeName(id); if (LOG.isDebugEnabled()) { LOG.debug( "Created id: " + id); } } catch (KeeperException e) { LOG.warn( "Caught: " + e, e); } catch (InterruptedException e) { LOG.warn( "Caught: " + e, e); } } zookeeper.create is not idempotent, so blindly retrying will land you into problems: 1) You start the create, a connection error happens, the create completes but you don't get a response 2) You retry your create 3) You may end up waiting on the znode from step 1) which will not go away I've been thinking of easy ways of getting around this problem and the easiest seems to be constructing the name as x-identifier-sequenceNumber. You could use the hostname or the sessionid as the identifier. When you do the retry you have to do a getChildren() to see if there are any znodes with your identifier and then use that znode if it exists.
          Hide
          james strachan added a comment -

          Great catch Benjamin! I've a working patch using your algorithm; am using x-sessionId-sequenceNumber and its working a treat (though its a tad hard to force ZK to fail mid-create .

          Am working on some unit tests to try out the server stopping/starting which I'll attach shortly once they're working a bit better...

          Show
          james strachan added a comment - Great catch Benjamin! I've a working patch using your algorithm; am using x-sessionId-sequenceNumber and its working a treat (though its a tad hard to force ZK to fail mid-create . Am working on some unit tests to try out the server stopping/starting which I'll attach shortly once they're working a bit better...
          Hide
          Hiram Chirino added a comment -

          If possible, it might be nice if the WriteLock implemented the Lock interface.

          Show
          Hiram Chirino added a comment - If possible, it might be nice if the WriteLock implemented the Lock interface.
          Hide
          james strachan added a comment -

          this modified patch includes an implementation of Benjamin's algorithm, using w-sessionId-sequenceNumber as a naming convention so that we can reuse files created for the same session if we get a connection failure.

          this patch also includes a unit test which tests out the WriteLock still working if we stop and start the server.

          also the code is refactored so that all the retry logic is in ProtocolSupport

          Show
          james strachan added a comment - this modified patch includes an implementation of Benjamin's algorithm, using w-sessionId-sequenceNumber as a naming convention so that we can reuse files created for the same session if we get a connection failure. this patch also includes a unit test which tests out the WriteLock still working if we stop and start the server. also the code is refactored so that all the retry logic is in ProtocolSupport
          Hide
          james strachan added a comment -

          this patch depends on the reconnect() method on ZooKeeper to deal with session expired exceptions

          Show
          james strachan added a comment - this patch depends on the reconnect() method on ZooKeeper to deal with session expired exceptions
          Hide
          james strachan added a comment -

          This patch no longer requires ZOOKEEPER-84, we now use a ZooKeeperFacade which wraps up the creation of the ZooKeeper instance and allows it to be replaced if a SessionExpiredException occurs.

          The test case works in the current patch. To get the test case to hang closing the 3rd client, just edit WriteLockTest and set the workAroundClosingLastZNodeFails field to a value of false. You will then get this stack dump when the test hangs (on OS X at least ...

              [junit] "main" prio=5 tid=0x01001710 nid=0xb0801000 in Object.wait() [0xb07ff000..0xb0800148]
              [junit]     at java.lang.Object.wait(Native Method)
              [junit]     - waiting on <0x096105e0> (a org.apache.zookeeper.ClientCnxn$Packet)
              [junit]     at java.lang.Object.wait(Object.java:474)
              [junit]     at org.apache.zookeeper.ClientCnxn.submitRequest(ClientCnxn.java:822)
              [junit]     - locked <0x096105e0> (a org.apache.zookeeper.ClientCnxn$Packet)
              [junit]     at org.apache.zookeeper.ZooKeeper.close(ZooKeeper.java:329)
              [junit]     - locked <0x0bd54108> (a org.apache.zookeeper.ZooKeeper)
              [junit]     at org.apache.zookeeper.protocols.ZooKeeperFacade.close(ZooKeeperFacade.java:99)
              [junit]     at org.apache.zookeeper.protocols.WriteLockTest.tearDown(WriteLockTest.java:146)
              [junit]     at junit.framework.TestCase.runBare(TestCase.java:140)
              [junit]     at junit.framework.TestResult$1.protect(TestResult.java:110)
              [junit]     at junit.framework.TestResult.runProtected(TestResult.java:128)
              [junit]     at junit.framework.TestResult.run(TestResult.java:113)
              [junit]     at junit.framework.TestCase.run(TestCase.java:124)
              [junit]     at junit.framework.TestSuite.runTest(TestSuite.java:232)
              [junit]     at junit.framework.TestSuite.run(TestSuite.java:227)
              [junit]     at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:81)
              [junit]     at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:36)
              [junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:421)
              [junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:912)
              [junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:766)
          

          This could maybe just be an OS X based timing issue

          Show
          james strachan added a comment - This patch no longer requires ZOOKEEPER-84 , we now use a ZooKeeperFacade which wraps up the creation of the ZooKeeper instance and allows it to be replaced if a SessionExpiredException occurs. The test case works in the current patch. To get the test case to hang closing the 3rd client, just edit WriteLockTest and set the workAroundClosingLastZNodeFails field to a value of false. You will then get this stack dump when the test hangs (on OS X at least ... [junit] "main" prio=5 tid=0x01001710 nid=0xb0801000 in Object .wait() [0xb07ff000..0xb0800148] [junit] at java.lang. Object .wait(Native Method) [junit] - waiting on <0x096105e0> (a org.apache.zookeeper.ClientCnxn$Packet) [junit] at java.lang. Object .wait( Object .java:474) [junit] at org.apache.zookeeper.ClientCnxn.submitRequest(ClientCnxn.java:822) [junit] - locked <0x096105e0> (a org.apache.zookeeper.ClientCnxn$Packet) [junit] at org.apache.zookeeper.ZooKeeper.close(ZooKeeper.java:329) [junit] - locked <0x0bd54108> (a org.apache.zookeeper.ZooKeeper) [junit] at org.apache.zookeeper.protocols.ZooKeeperFacade.close(ZooKeeperFacade.java:99) [junit] at org.apache.zookeeper.protocols.WriteLockTest.tearDown(WriteLockTest.java:146) [junit] at junit.framework.TestCase.runBare(TestCase.java:140) [junit] at junit.framework.TestResult$1.protect(TestResult.java:110) [junit] at junit.framework.TestResult.runProtected(TestResult.java:128) [junit] at junit.framework.TestResult.run(TestResult.java:113) [junit] at junit.framework.TestCase.run(TestCase.java:124) [junit] at junit.framework.TestSuite.runTest(TestSuite.java:232) [junit] at junit.framework.TestSuite.run(TestSuite.java:227) [junit] at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:81) [junit] at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:36) [junit] at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:421) [junit] at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:912) [junit] at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:766) This could maybe just be an OS X based timing issue
          Hide
          james strachan added a comment -

          Patch is now attached

          Show
          james strachan added a comment - Patch is now attached
          Hide
          Benjamin Reed added a comment -

          Fantastic work! I second Hiram's suggestion to implement the Lock interface. (You could probably throw unsupported exception for the condition method.) It would make your naming more symmetric anyway

          I love the whenOwner field! Excellent idea. I think it would be good to allow it to be passed in the constructor.

          I have a problem with your use of the facade. Here is the problematic scenario: your application waits for a lock and once acquired becomes the master. This master thread services clients but more importantly updates stuff in ZooKeeper. If the session expires and reconnects automatically, it is very easy for your master thread to make changes without actually being a master using the new session. If you don't use the facade, things fail properly: the connection expires, the master is no longer the master in ZooKeeper, and (most importantly) the master thread cannot do any ZooKeeper operations with the ZooKeeper object.

          I realize the facade may seem convenient, but I think we need to encourage safe behavior especially with the first high level primitives and since yours will be the very first I'd like to make sure we get off to a safe start. (It's a great first example by the way!)

          Show
          Benjamin Reed added a comment - Fantastic work! I second Hiram's suggestion to implement the Lock interface. (You could probably throw unsupported exception for the condition method.) It would make your naming more symmetric anyway I love the whenOwner field! Excellent idea. I think it would be good to allow it to be passed in the constructor. I have a problem with your use of the facade. Here is the problematic scenario: your application waits for a lock and once acquired becomes the master. This master thread services clients but more importantly updates stuff in ZooKeeper. If the session expires and reconnects automatically, it is very easy for your master thread to make changes without actually being a master using the new session. If you don't use the facade, things fail properly: the connection expires, the master is no longer the master in ZooKeeper, and (most importantly) the master thread cannot do any ZooKeeper operations with the ZooKeeper object. I realize the facade may seem convenient, but I think we need to encourage safe behavior especially with the first high level primitives and since yours will be the very first I'd like to make sure we get off to a safe start. (It's a great first example by the way!)
          Hide
          james strachan added a comment -

          Moving patches for this issue to subversion for easier tracking

          http://svn.apache.org/repos/asf/activemq/sandbox/zookeeper/zookeeper-protocols/

          I've already submitted about five patches for this issue so far and I'm sure there's gonna be loads more coming. Developing higher level protocols is a much bigger job than I previously thought particularly with having tests for all the various failure scenarios and adding support for the various other higher level protocols.

          Its kinda time consuming creating loads of patches & attaching them to the same issue and deleting the old ones so its easy for commmitters to review - but more importantly, all the history of the many patches gets totally lost using the attach-patch-to-jira model - which also makes it harder for committers to watch progress on this issue.

          I've never done this before on any other Apache project - and this approach is temporary and only reserved for the single ZOOKEEPER-78 issue; but I've checked in this patch into an svn sandbox area at Apache that I have commit karma on and will continue to work on it there; so that all the history is preserved. I can then do many more frequent & smaller commits; any ZK committer can review and easily apply my patches whenever they feel like - and its gonna be much easier for anyone in the ZK community to track progress on this issue and see how the implementation has changed over time as some things work or I find better ways of solving the issue.

          This approach is totally temporary; its not an attempt to move the code outside of the ZK community or anything like that. At any point feel free to commit (actually just copy in svn which will keep all the history & commit comments etc) to the ZK trunk. You could even mirror the code to the ZK tree in sandbox/contrib area if you like - just like Hiram did to mirror the ZK code to the maven-patch example in the activemq sandbox.

          I'm hoping in a few weeks my hacking on this issue will near completion and we can permanently move the code back into the ZK tree; but in the meantime its trivial to reuse it where it is or mirror it into the ZK tree as folks in the ZK community see fit. Also if I ever earn committer karma on ZK I can just move it into some ZK contrib area myself

          Building the code

          In terms of sandbox - I ended up reusing Hiram's sandbox area that shows the maven build working on ZK; as I prefer to use maven and it was then super easy for me to create a new maven module, zookeeper-protocols that just includes the source and test cases for the high level protocols.

          If you're new to maven and want to build it, I've checked in instructions here...
          https://svn.apache.org/repos/asf/activemq/sandbox/zookeeper/BUILDING-maven.txt

          Whenever we move this code back into the ZK trunk am sure we can hack an Ant build for it.

          Show
          james strachan added a comment - Moving patches for this issue to subversion for easier tracking http://svn.apache.org/repos/asf/activemq/sandbox/zookeeper/zookeeper-protocols/ I've already submitted about five patches for this issue so far and I'm sure there's gonna be loads more coming. Developing higher level protocols is a much bigger job than I previously thought particularly with having tests for all the various failure scenarios and adding support for the various other higher level protocols. Its kinda time consuming creating loads of patches & attaching them to the same issue and deleting the old ones so its easy for commmitters to review - but more importantly, all the history of the many patches gets totally lost using the attach-patch-to-jira model - which also makes it harder for committers to watch progress on this issue. I've never done this before on any other Apache project - and this approach is temporary and only reserved for the single ZOOKEEPER-78 issue; but I've checked in this patch into an svn sandbox area at Apache that I have commit karma on and will continue to work on it there; so that all the history is preserved. I can then do many more frequent & smaller commits; any ZK committer can review and easily apply my patches whenever they feel like - and its gonna be much easier for anyone in the ZK community to track progress on this issue and see how the implementation has changed over time as some things work or I find better ways of solving the issue. This approach is totally temporary; its not an attempt to move the code outside of the ZK community or anything like that. At any point feel free to commit (actually just copy in svn which will keep all the history & commit comments etc) to the ZK trunk. You could even mirror the code to the ZK tree in sandbox/contrib area if you like - just like Hiram did to mirror the ZK code to the maven-patch example in the activemq sandbox. I'm hoping in a few weeks my hacking on this issue will near completion and we can permanently move the code back into the ZK tree; but in the meantime its trivial to reuse it where it is or mirror it into the ZK tree as folks in the ZK community see fit. Also if I ever earn committer karma on ZK I can just move it into some ZK contrib area myself Building the code In terms of sandbox - I ended up reusing Hiram's sandbox area that shows the maven build working on ZK; as I prefer to use maven and it was then super easy for me to create a new maven module, zookeeper-protocols that just includes the source and test cases for the high level protocols. If you're new to maven and want to build it, I've checked in instructions here... https://svn.apache.org/repos/asf/activemq/sandbox/zookeeper/BUILDING-maven.txt Whenever we move this code back into the ZK trunk am sure we can hack an Ant build for it.
          Hide
          james strachan added a comment -

          Thanks for the great comments Benjamin! Have already added the constructor for you

          BTW I was pondering about switching the whenOwner from a Runnable to some kinda interface that invokes you when you become the leader/owner - or when you stop being the leader/owner. Something like

          public interface WhenOwnerListener {
            void whenOwner();
            void whenNotOwner();
          }
          

          Where only znodes that are the owner would be notified with the whenOwner() method; but then if a connection fails or session times out, they'd be notified with a call to whenNotOwner();

          Spookily - I'd set myself the target today to properly implement the watches so that WriteLock gets a notification of it no longer being the leader/owner when a connection fails (which normally auto-reconnects anyway right now in the base ZooKeeper). Then I was gonna add a notification mechanism so we could notify the leader/owner is no longer the leader/owner when the session expired exception occurs.

          So we're absolutely on the same page; once I'd grokked the proper watch code for dealing with normal connection failures & reconnects I was hoping to add something vaguely similar to the ZooKeeperFacade so that higher level protocols can be aware of both when ZooKeeper reconnects and when ZooKeeperFacade creates a whole new connection.

          Does that make sense? I totally understand your concerns at making sure the WriteLock and associated helper classes like ProtocolSupport/ZooKeeperFacade do the right thing - I want exactly the same thing I'd just not yet had the chance to go through all the different failure conditions and scenarios and make sure they all work properly

          Show
          james strachan added a comment - Thanks for the great comments Benjamin! Have already added the constructor for you BTW I was pondering about switching the whenOwner from a Runnable to some kinda interface that invokes you when you become the leader/owner - or when you stop being the leader/owner. Something like public interface WhenOwnerListener { void whenOwner(); void whenNotOwner(); } Where only znodes that are the owner would be notified with the whenOwner() method; but then if a connection fails or session times out, they'd be notified with a call to whenNotOwner(); Spookily - I'd set myself the target today to properly implement the watches so that WriteLock gets a notification of it no longer being the leader/owner when a connection fails (which normally auto-reconnects anyway right now in the base ZooKeeper). Then I was gonna add a notification mechanism so we could notify the leader/owner is no longer the leader/owner when the session expired exception occurs. So we're absolutely on the same page; once I'd grokked the proper watch code for dealing with normal connection failures & reconnects I was hoping to add something vaguely similar to the ZooKeeperFacade so that higher level protocols can be aware of both when ZooKeeper reconnects and when ZooKeeperFacade creates a whole new connection. Does that make sense? I totally understand your concerns at making sure the WriteLock and associated helper classes like ProtocolSupport/ZooKeeperFacade do the right thing - I want exactly the same thing I'd just not yet had the chance to go through all the different failure conditions and scenarios and make sure they all work properly
          Hide
          james strachan added a comment -

          Just added the WhenOwnerListener interface : http://svn.apache.org/viewvc?view=rev&revision=679325 I just need to figure out how to add notifications of loss of owner/leader status when the connection fails or the session expires etc.

          Show
          james strachan added a comment - Just added the WhenOwnerListener interface : http://svn.apache.org/viewvc?view=rev&revision=679325 I just need to figure out how to add notifications of loss of owner/leader status when the connection fails or the session expires etc.
          Hide
          james strachan added a comment -

          Benjamin I added ZOOKEEPER-89 and ZOOKEEPER-90 to track the dealing with loss of ownership/leader with connection reconnects and with session expiration. I've not been able to test out the latter yet; but I've tested the former and I think both are implemented now via the patch for
          ZOOKEEPER-90 and ZOOKEEPER-89

          Show
          james strachan added a comment - Benjamin I added ZOOKEEPER-89 and ZOOKEEPER-90 to track the dealing with loss of ownership/leader with connection reconnects and with session expiration. I've not been able to test out the latter yet; but I've tested the former and I think both are implemented now via the patch for ZOOKEEPER-90 and ZOOKEEPER-89
          Hide
          Patrick Hunt added a comment -

          Hi James – I understand where you are coming from and we appreciate the effort, but regarding use of SVN for patch submission - I'm afraid this is not acceptable. Notice on the "attach patch" JIRA page that it has the " Grant license to ASF for inclusion in ASF works .... " option. This has to be checked for us to consider a patch for inclusion.

          Also consider that we have limited resources/time and need to follow the designated patch process in order to optimize around the limited set of core reviewers/commiters we have relative to the community at large.

          (if you think you've got it bad checkout some of the hadoop examples like: https://issues.apache.org/jira/browse/HADOOP-3412)

          What I'd suggest is that you "overwrite" the existing patch when submitting an update. If you upload a file with the same name the old version of the file is still available in the JIRA, and the new file will take it's place. (I need to update the wiki with this, just haven't gotten around to it - ok just added near the end of the "contributing your work section", thanks)

          A bit less important but wanted to mention the naming scheme defined on the following page (creating a patch section):
          http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute
          ZOOKEEPER-#.patch
          This helps reviewers/committers keep all the patches straight.

          Show
          Patrick Hunt added a comment - Hi James – I understand where you are coming from and we appreciate the effort, but regarding use of SVN for patch submission - I'm afraid this is not acceptable. Notice on the "attach patch" JIRA page that it has the " Grant license to ASF for inclusion in ASF works .... " option. This has to be checked for us to consider a patch for inclusion. Also consider that we have limited resources/time and need to follow the designated patch process in order to optimize around the limited set of core reviewers/commiters we have relative to the community at large. (if you think you've got it bad checkout some of the hadoop examples like: https://issues.apache.org/jira/browse/HADOOP-3412 ) What I'd suggest is that you "overwrite" the existing patch when submitting an update. If you upload a file with the same name the old version of the file is still available in the JIRA, and the new file will take it's place. (I need to update the wiki with this, just haven't gotten around to it - ok just added near the end of the "contributing your work section", thanks) A bit less important but wanted to mention the naming scheme defined on the following page (creating a patch section): http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute ZOOKEEPER-#.patch This helps reviewers/committers keep all the patches straight.
          Hide
          Doug Cutting added a comment -

          The project's patch submission mechanism is not inflexible, but neither should it be changed unilaterally on an issue-by-issue basis. The currently acceptable mechanism is to attach patch files to Jira, generated by 'svn diff'. When files are renamed, a shell script should be provided that performs the renames, where the script is run before the patch is applied. If some find this awkward, then a separate discussion should be launched on the mailing list. Some projects are now, e.g., using 'git' to handle things like this, but this needs to be done carefully so that the entire community is included in the process. Fewer, simpler mechanisms generally include more people.

          Show
          Doug Cutting added a comment - The project's patch submission mechanism is not inflexible, but neither should it be changed unilaterally on an issue-by-issue basis. The currently acceptable mechanism is to attach patch files to Jira, generated by 'svn diff'. When files are renamed, a shell script should be provided that performs the renames, where the script is run before the patch is applied. If some find this awkward, then a separate discussion should be launched on the mailing list. Some projects are now, e.g., using 'git' to handle things like this, but this needs to be done carefully so that the entire community is included in the process. Fewer, simpler mechanisms generally include more people.
          Hide
          Hiram Chirino added a comment - - edited

          Hi Patrick,

          The "Notice on the "attach patch" JIRA page that it has the " Grant license to ASF for inclusion in ASF works .... " option. This has to be checked for us to consider a patch for inclusion. " is not accurate in this case.

          James and I are are both Apache committers and Members, and as such, when we commit code to the ASF repository a license is granted to the ASF. The jira feature is really only there to be able to accept code from folks who have not filed an ICLA with the ASF.

          Another way to view this development model is as if we were ZooKeeper committers who do not commit to trunk but which develop new features and bug fixes in development branches. This model of development is use extensively in projects who are adverse to destabilizing the trunk. They develop and test new features in a branch and then merge back once folks are happy with it.

          This model is also outlined at Rules for Revolutionaries

          Show
          Hiram Chirino added a comment - - edited Hi Patrick, The "Notice on the "attach patch" JIRA page that it has the " Grant license to ASF for inclusion in ASF works .... " option. This has to be checked for us to consider a patch for inclusion. " is not accurate in this case. James and I are are both Apache committers and Members, and as such, when we commit code to the ASF repository a license is granted to the ASF. The jira feature is really only there to be able to accept code from folks who have not filed an ICLA with the ASF. Another way to view this development model is as if we were ZooKeeper committers who do not commit to trunk but which develop new features and bug fixes in development branches. This model of development is use extensively in projects who are adverse to destabilizing the trunk. They develop and test new features in a branch and then merge back once folks are happy with it. This model is also outlined at Rules for Revolutionaries
          Hide
          james strachan added a comment -

          Wow I confess to be being kinda surprised by that response I didn't realise you guys were so attached to the exact svn command line used to apply a patch - I thought you'd welcome all contributions and that the svn commit history would be more useful to you, given the large amount of changes and complexity of the code and large number of comments already on this JIRA - rather than focus purely on a minor couple of keystrokes required apply the patch

          FWIW I've attached about 5 patch files so far, with none of them being committed anywhere - then made about 7 patches since then in svn with history and I'm sure there'll be another 5 or so changes to go before this patch is done.

          Never mind - I'll happily comply with your strict patch acceptance policy. Give me a few weeks or so to completely finish the code and documentation and I'll submit a single big patch for all the work to this JIRA. If you want you can get all the history too with a trivial alternative svn command - but if that offends you, please forget I'm using a sandbox svn area to work on this (pretend I'm just saving it on my hard drive and please disregard the links I've added to some JIRAs to refer to parts of this patch in a simple way) and just use the single patch file I'll attach in a few weeks or so.

          Show
          james strachan added a comment - Wow I confess to be being kinda surprised by that response I didn't realise you guys were so attached to the exact svn command line used to apply a patch - I thought you'd welcome all contributions and that the svn commit history would be more useful to you, given the large amount of changes and complexity of the code and large number of comments already on this JIRA - rather than focus purely on a minor couple of keystrokes required apply the patch FWIW I've attached about 5 patch files so far, with none of them being committed anywhere - then made about 7 patches since then in svn with history and I'm sure there'll be another 5 or so changes to go before this patch is done. Never mind - I'll happily comply with your strict patch acceptance policy. Give me a few weeks or so to completely finish the code and documentation and I'll submit a single big patch for all the work to this JIRA. If you want you can get all the history too with a trivial alternative svn command - but if that offends you, please forget I'm using a sandbox svn area to work on this (pretend I'm just saving it on my hard drive and please disregard the links I've added to some JIRAs to refer to parts of this patch in a simple way) and just use the single patch file I'll attach in a few weeks or so.
          Hide
          Nigel Daley added a comment -

          Fewer, simpler mechanisms generally include more people.

          +1. Let's stick with a process for now that all contributors can use, not just ASF committers.

          Show
          Nigel Daley added a comment - Fewer, simpler mechanisms generally include more people. +1. Let's stick with a process for now that all contributors can use, not just ASF committers.
          Hide
          james strachan added a comment -

          Let's stick with a process for now that all contributors can use, not just ASF committers.

          Huh? Everyone has access to ASF svn? Only committers can commit using either approach. I don't grok your point.

          Show
          james strachan added a comment - Let's stick with a process for now that all contributors can use, not just ASF committers. Huh? Everyone has access to ASF svn? Only committers can commit using either approach. I don't grok your point.
          Hide
          Doug Cutting added a comment -

          > Give me a few weeks or so to completely finish the code and documentation and I'll submit a single big patch for all the work to this JIRA.

          If you want to get more feedback as you go, please attach interim patches too, rather than just the final patch. That way folks who monitor the -dev mailing list will see the updates and can try them in the usual manner. We generally don't put things into the "Patch Available" state until the contributor believes that the patch is in a final form.

          > Everyone has access to ASF svn?

          I think all that Nigel meant was that not everyone can submit patches by naming an Apache svn revision since not everyone has commit access to Apache's svn. We try to use a uniform mechanism, the same for committers as non-committers. Committers generally have to do more work, not less, than other contributors, since every patch committed must be reviewed and accepted by a committer. Committing is a duty, not a privilege.

          Show
          Doug Cutting added a comment - > Give me a few weeks or so to completely finish the code and documentation and I'll submit a single big patch for all the work to this JIRA. If you want to get more feedback as you go, please attach interim patches too, rather than just the final patch. That way folks who monitor the -dev mailing list will see the updates and can try them in the usual manner. We generally don't put things into the "Patch Available" state until the contributor believes that the patch is in a final form. > Everyone has access to ASF svn? I think all that Nigel meant was that not everyone can submit patches by naming an Apache svn revision since not everyone has commit access to Apache's svn. We try to use a uniform mechanism, the same for committers as non-committers. Committers generally have to do more work, not less, than other contributors, since every patch committed must be reviewed and accepted by a committer. Committing is a duty, not a privilege.
          Hide
          Doug Cutting added a comment -

          > FWIW I've attached about 5 patch files so far, with none of them being committed anywhere [ ...]

          Are they getting reviewed promptly? A primary job of committers is to keep the Patch Available queue short, by trying to review patches within a few days of their contribution, either committing them or rejecting them with a clear explanation. If the committers are unable to keep up with the contributors, then the project probably needs more committers. In this case, contributors should also do everything they can to make committers lives easier, so as not to further delay their patches.

          Contributors should be nominated to become committers when they've provided a series (e.g., 3+) non-trivial, high-quality patches, and demonstrated an ability to work peacefully with existing committers, following procedures, etc. If someone provides patches that apply cleanly, fix real user problems, include unit tests, and the contributor responds to criticism in a positive manner, then they should be nominated as a committer in short order. Then they can start reviewing and grooming new committers themselves!

          Show
          Doug Cutting added a comment - > FWIW I've attached about 5 patch files so far, with none of them being committed anywhere [ ...] Are they getting reviewed promptly? A primary job of committers is to keep the Patch Available queue short, by trying to review patches within a few days of their contribution, either committing them or rejecting them with a clear explanation. If the committers are unable to keep up with the contributors, then the project probably needs more committers. In this case, contributors should also do everything they can to make committers lives easier, so as not to further delay their patches. Contributors should be nominated to become committers when they've provided a series (e.g., 3+) non-trivial, high-quality patches, and demonstrated an ability to work peacefully with existing committers, following procedures, etc. If someone provides patches that apply cleanly, fix real user problems, include unit tests, and the contributor responds to criticism in a positive manner, then they should be nominated as a committer in short order. Then they can start reviewing and grooming new committers themselves!
          Hide
          Patrick Hunt added a comment -

          I agree, we do need to do a better job on the reviews. In our defense:

          1) we're trying to complete the migration from SF to Apache. good thing is it looks like we're pretty close to the end on that
          2) there's a ZooKeeper workshop @yahoo on Monday, just so happens that all commiters are presenters.
          3) the continual push to change the build/wiki/patchsubmission/etc... processes are pulling me off things like patch review in order to track down answers (cuz I'm new to Apache as well - thanks Doug/Owen!)

          Show
          Patrick Hunt added a comment - I agree, we do need to do a better job on the reviews. In our defense: 1) we're trying to complete the migration from SF to Apache. good thing is it looks like we're pretty close to the end on that 2) there's a ZooKeeper workshop @yahoo on Monday, just so happens that all commiters are presenters. 3) the continual push to change the build/wiki/patchsubmission/etc... processes are pulling me off things like patch review in order to track down answers (cuz I'm new to Apache as well - thanks Doug/Owen!)
          Show
          Patrick Hunt added a comment - Per the comments here: https://issues.apache.org/jira/browse/ZOOKEEPER-78?focusedCommentId=12616579#action_12616579 and here: https://issues.apache.org/jira/browse/ZOOKEEPER-78?focusedCommentId=12616624#action_12616624 I'm moving the status from patch available to open.
          Hide
          Benjamin Reed added a comment -

          Can we focus on the issue at hand? This patch is almost done there is no reason for the meta physical discussions or even an external SVN. It's simple enough. The only thing that must be done is to remove the use of the ZooKeeperFacade and use the ZooKeeper object correctly. If you want to discuss why ZooKeeperFacade is a bad idea, lets open an issue to discuss there.

          I really like the idea of using the Lock interface, but changing aquire to lock() should be good enough.

          Lets get this thing committed so that we can move on.

          Show
          Benjamin Reed added a comment - Can we focus on the issue at hand? This patch is almost done there is no reason for the meta physical discussions or even an external SVN. It's simple enough. The only thing that must be done is to remove the use of the ZooKeeperFacade and use the ZooKeeper object correctly. If you want to discuss why ZooKeeperFacade is a bad idea, lets open an issue to discuss there. I really like the idea of using the Lock interface, but changing aquire to lock() should be good enough. Lets get this thing committed so that we can move on.
          Hide
          Benjamin Reed added a comment -

          I've documented on the wiki at http://wiki.apache.org/hadoop/ZooKeeper/ErrorHandling why ZooKeeperFacade needs to be removed. Just use the ZooKeeper object. For this patch in particular it is very important not to use a facade.

          Show
          Benjamin Reed added a comment - I've documented on the wiki at http://wiki.apache.org/hadoop/ZooKeeper/ErrorHandling why ZooKeeperFacade needs to be removed. Just use the ZooKeeper object. For this patch in particular it is very important not to use a facade.
          Hide
          Mahadev konar added a comment -

          it would be great ot have this jira in 3.2.

          this patch removes the zookeeper facade, and makes it work with the current trunk. I have to still go through all the corner cases and see if they have been handled. Also, I need to implement the lock interface in writelock.

          comments are welcome...

          Show
          Mahadev konar added a comment - it would be great ot have this jira in 3.2. this patch removes the zookeeper facade, and makes it work with the current trunk. I have to still go through all the corner cases and see if they have been handled. Also, I need to implement the lock interface in writelock. comments are welcome...
          Hide
          Mahadev konar added a comment -

          an updated patch.

          • added acls to writelock
          • removed ZookeeperTestSupport, used ClientBase insted of this new class
          • made the package name org.apache.zookepeer.protocols.lock so that each protocol has its own direcotry
          • moved the docs.html to pakcage.html

          it still does not impleement the lock interface of java. Ill add it in the next patch.

          Show
          Mahadev konar added a comment - an updated patch. added acls to writelock removed ZookeeperTestSupport, used ClientBase insted of this new class made the package name org.apache.zookepeer.protocols.lock so that each protocol has its own direcotry moved the docs.html to pakcage.html it still does not impleement the lock interface of java. Ill add it in the next patch.
          Hide
          Tom White added a comment -

          Mahadev,

          I'm glad this is being worked on again. Some comments:

          • Not sure if a Runnable is right for whenOwner, as Runnable implementations are often one-shot and can't be re-run, whereas it looks like WriteLock can be locked and unlocked repeatedly (is this right?). There's also no way to tell when the lock has been released. Might be better to have a standard listener interface called LockListener, with lockAcquired and lockReleased methods. Writing an implementation to run a Runnable would be trivial.
          • I worry a little about WriteLock implementing java.util.concurrent.locks.Lock, since Lock seems oriented to single-process programs. By making WriteLock a Lock we make it easy for programmers to drop it in to programs that use Lock, without having to think about distributed error recovery. For example, Lock#lock() cannot throw checked exceptions, so we would have to wrap KeeperException in an unchecked exception, which could too readily be ignored. Perhaps the answer is to make the method naming and semantics as close as possible, without actually implementing Lock proper? (I think this is similar to the discussion in ZOOKEEPER-104.)
          • Lots of classes and methods are public when they don't need to be. Really only WriteLock should be public, with public methods lock(), unlock(), getDir() (better named as getLockPath() or similar?), getWhenOwner(), setWhenOwner() (or whatever replaces the last two).
          Show
          Tom White added a comment - Mahadev, I'm glad this is being worked on again. Some comments: Not sure if a Runnable is right for whenOwner, as Runnable implementations are often one-shot and can't be re-run, whereas it looks like WriteLock can be locked and unlocked repeatedly (is this right?). There's also no way to tell when the lock has been released. Might be better to have a standard listener interface called LockListener, with lockAcquired and lockReleased methods. Writing an implementation to run a Runnable would be trivial. I worry a little about WriteLock implementing java.util.concurrent.locks.Lock, since Lock seems oriented to single-process programs. By making WriteLock a Lock we make it easy for programmers to drop it in to programs that use Lock, without having to think about distributed error recovery. For example, Lock#lock() cannot throw checked exceptions, so we would have to wrap KeeperException in an unchecked exception, which could too readily be ignored. Perhaps the answer is to make the method naming and semantics as close as possible, without actually implementing Lock proper? (I think this is similar to the discussion in ZOOKEEPER-104 .) Lots of classes and methods are public when they don't need to be. Really only WriteLock should be public, with public methods lock(), unlock(), getDir() (better named as getLockPath() or similar?), getWhenOwner(), setWhenOwner() (or whatever replaces the last two).
          Hide
          Mahadev konar added a comment -

          sorry for my late response tom.. i havent had a real close look at the interfaces and methods in this patch myself, so thanks for reviewing.. I was mainly looking at the handling of zookeeper events.

          1) I think you are rught that we should probably have call back methods with lockacruired and lockreleeased methods. The current implementation is too restrictive.
          2) I am with you on this one as well... I hadn't implemented the lock interface just because I had the same reservations as you.. I think for now we should just leave it as it is without implementing the lock interface and see what our users have to say..
          3) agreed..

          Show
          Mahadev konar added a comment - sorry for my late response tom.. i havent had a real close look at the interfaces and methods in this patch myself, so thanks for reviewing.. I was mainly looking at the handling of zookeeper events. 1) I think you are rught that we should probably have call back methods with lockacruired and lockreleeased methods. The current implementation is too restrictive. 2) I am with you on this one as well... I hadn't implemented the lock interface just because I had the same reservations as you.. I think for now we should just leave it as it is without implementing the lock interface and see what our users have to say.. 3) agreed..
          Hide
          Mahadev konar added a comment -

          a new patch. I have changed the directory structure in this

          the code now is in
          root/src/recipes/lock/src/java/org/apache/zookepeer/recipes/lock/
          also the tests are in
          root/src/recipes/lock/test/org/apache/zookeeper/recipes/lock/

          the new directory structure allows us to have both the java and c implemention in the same parent directory strucure.

          src/recipes/lock/

          also -

          • added a new interface Locklistener
          • removed runnable to be a locklistener interface whose methods lockAcqured and lockReleased are called on a lock acquired and relase of a lock
          • refactored some code
          • deleted not required public methods.
          • added build files for the recipes directory
          • changed the tests to work with new api's

          i am implementing the recipes in c. Will have an updated patch up soon. comments are welcome.

          Show
          Mahadev konar added a comment - a new patch. I have changed the directory structure in this the code now is in root /src/recipes/lock/src/java/org/apache/zookepeer/recipes/lock/ also the tests are in root /src/recipes/lock/test/org/apache/zookeeper/recipes/lock/ the new directory structure allows us to have both the java and c implemention in the same parent directory strucure. src/recipes/lock/ also - added a new interface Locklistener removed runnable to be a locklistener interface whose methods lockAcqured and lockReleased are called on a lock acquired and relase of a lock refactored some code deleted not required public methods. added build files for the recipes directory changed the tests to work with new api's i am implementing the recipes in c. Will have an updated patch up soon. comments are welcome.
          Hide
          Tom White added a comment -

          > i am implementing the recipes in c.

          Should the C version go in a separate Jira issue and patch?

          Show
          Tom White added a comment - > i am implementing the recipes in c. Should the C version go in a separate Jira issue and patch?
          Hide
          Mahadev konar added a comment -

          dont know.. the jira says just talks abt the recipe, so is adequate for both java and c . .. I would like to have both in one jira so that everyone who wants to contribute to such zookeeeper recipes is encouraged to have both the java and c implementation, but I am open to creating a new jira for c.

          Show
          Mahadev konar added a comment - dont know.. the jira says just talks abt the recipe, so is adequate for both java and c . .. I would like to have both in one jira so that everyone who wants to contribute to such zookeeeper recipes is encouraged to have both the java and c implementation, but I am open to creating a new jira for c.
          Hide
          Patrick Hunt added a comment -

          I agree with Mahadev – we want to encourage ppl to provide (interoperable) implementations for both c/java.

          However the JIRA should be updated to have both java and c client component listed.

          Should we add a new "recipe" component?

          Show
          Patrick Hunt added a comment - I agree with Mahadev – we want to encourage ppl to provide (interoperable) implementations for both c/java. However the JIRA should be updated to have both java and c client component listed. Should we add a new "recipe" component?
          Hide
          Mahadev konar added a comment -

          this patch has the c library in it as well.
          Now I think of it, I probably should have done it in a seperate jira with subtasks as java and c libraries.

          • added the c library with auto * files to create teh library
          • added cpp unit testing for the c library
          • similar to java interface, the c interface also allows a callback method to be called in case of lock being avquired and released.
          • i will be cleaning up the patch (with some more docs and rmeocing unneccasry printf's and unused code).

          comments are welcome...

          Show
          Mahadev konar added a comment - this patch has the c library in it as well. Now I think of it, I probably should have done it in a seperate jira with subtasks as java and c libraries. added the c library with auto * files to create teh library added cpp unit testing for the c library similar to java interface, the c interface also allows a callback method to be called in case of lock being avquired and released. i will be cleaning up the patch (with some more docs and rmeocing unneccasry printf's and unused code). comments are welcome...
          Hide
          Mahadev konar added a comment -

          reattaching the patch. had some problems with the earler version.

          Show
          Mahadev konar added a comment - reattaching the patch. had some problems with the earler version.
          Hide
          Mahadev konar added a comment -

          make sure that you download the patch adn then view it. I tried opening up the patch in the browser and it fails. It thinks that the file is xml (not sure why.. ) and tried opening it as an xml file...

          Show
          Mahadev konar added a comment - make sure that you download the patch adn then view it. I tried opening up the patch in the browser and it fails. It thinks that the file is xml (not sure why.. ) and tried opening it as an xml file...
          Hide
          Chris Darroch added a comment -

          Just took a quick peek to see what would be either rework these in a ZooKeeper::Lock Perl module to go along with Net::ZooKeeper, or to expose the C version through Net::ZooKeeper. I may well be missing something after such a brief review, but in zoo_mutex_operation() I would be inclined to add some return value checks.

          For example, if zoo_create() here returns a failure code, I believe it's possible for the retbuf to contain whatever random data it had before the call. In that case, getName() might "succeed" if the data contains a '/' and return a string — possibly one that doesn't happen to be null-terminated within the retbuf buffer.

                     char retbuf[len+20];
                     snprintf(buf, len, "%s/%s", path, prefix);
                     ret = zoo_create(zh, buf, NULL, 0,  mutex->acl, 
                                      ZOO_EPHEMERAL|ZOO_SEQUENCE, retbuf, (len+20));
                     mutex->id = getName(retbuf);
          

          There are some calls to zoo_get_children(), etc., which I think might be usefully checked for failure return codes as well. Sorry not to provide a patch yet.

          Show
          Chris Darroch added a comment - Just took a quick peek to see what would be either rework these in a ZooKeeper::Lock Perl module to go along with Net::ZooKeeper, or to expose the C version through Net::ZooKeeper. I may well be missing something after such a brief review, but in zoo_mutex_operation() I would be inclined to add some return value checks. For example, if zoo_create() here returns a failure code, I believe it's possible for the retbuf to contain whatever random data it had before the call. In that case, getName() might "succeed" if the data contains a '/' and return a string — possibly one that doesn't happen to be null-terminated within the retbuf buffer. char retbuf[len+20]; snprintf(buf, len, "%s/%s", path, prefix); ret = zoo_create(zh, buf, NULL, 0, mutex->acl, ZOO_EPHEMERAL|ZOO_SEQUENCE, retbuf, (len+20)); mutex->id = getName(retbuf); There are some calls to zoo_get_children(), etc., which I think might be usefully checked for failure return codes as well. Sorry not to provide a patch yet.
          Hide
          Mahadev konar added a comment -

          thanks for your comments chris. You are right, the c version needs to do a better job on handling error cases. I still need to clean it up for that. Will update the patch. I probably think i spent most of the time fighting/learning auto*/libtools rather than coding the c api ...

          Show
          Mahadev konar added a comment - thanks for your comments chris. You are right, the c version needs to do a better job on handling error cases. I still need to clean it up for that. Will update the patch. I probably think i spent most of the time fighting/learning auto*/libtools rather than coding the c api ...
          Hide
          Mahadev konar added a comment -
          • cleaned up the code and added some mroe comments to the code.
          • made the java api be lock and unlock compatible with java's lock api

          I still need to work on doing the right handling of session expiration and connectionloss everywhere in the code (both c and java)

          Show
          Mahadev konar added a comment - cleaned up the code and added some mroe comments to the code. made the java api be lock and unlock compatible with java's lock api I still need to work on doing the right handling of session expiration and connectionloss everywhere in the code (both c and java)
          Hide
          Mahadev konar added a comment -

          this patch takes care of all the error cases (please review in case i missed some) both in java and c.

          Show
          Mahadev konar added a comment - this patch takes care of all the error cases (please review in case i missed some) both in java and c.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 39 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 116 release audit warnings (more than the trunk's current 105 warnings).

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/artifact/trunk/current/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/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/12404904/ZOOKEEPER-78.patch against trunk revision 762602. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 39 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 116 release audit warnings (more than the trunk's current 105 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/21/console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          This first set of comments is relative to the structure/build/packaging (ie non-code) issues:

          I think the jar name should include "recipes"
          ie for locks: zookeeper-3.2.0-recipes-lock.jar
          how about a "superset" recipes/zookeeper-3.2.0-recipes.jar with all recipes included?
          users can choose which to use

          There seems to be an issue with recipes in release build - actually same issue with contrib:
          you cannot run "ant test" on lock recipe in the released tar file build using "ant tar"
          seems build.xml and build-recipes.xml are missing from tar file

          docs - I think it's ok not to have specific forrest docs inside locsk, but need:
          recipes/README.txt
          what is this code? what are requirements for authors? (like interop btw c/java for particular impl)
          also req that recipe be documented in recipes documentation in forrest
          describe and point to forrest docs in docs/... (should put link to live h.a.o doc as well)
          is there some high level bit of informatin that you want to convey to authors re how components
          should be designed (in particular session mgmt, error handling, logging, etc...) this is a good place to put
          recipes/lock/README.txt
          describe and point to forrest docs in docs/... (should put link to live h.a.o doc as well)
          pointer to javadoc (ie live h.a.o site)?

          are the forrest recipe docs up to date relative to impl for locks? which lock in particular from docs.
          any deviation(s), any specific impl detail that's important, configurability?
          might be good to update the forrest recipe docs to point to this jar/code for implementation
          ie let users know that they can get this from the release in recipes/locks (they don't need to impl)

          how do we verify interop?
          great job adding tests! but they seem to be tests for c, tests for java, but not interop.
          this seems pretty important. We want to document in recipes/readme that authors in particular
          should code for c/java to interop for any one particular recipe instance. also that recipes can
          co-exists in user space (ie placement of nodes, etc...)
          Might be good enough to add a new JIRA for this, but need to impl this asap.

          You've done a great job, but I'm holding this jira to a higher standard since ppl who implement further
          recipes will use your lock implementation as a "blueprint".

          Show
          Patrick Hunt added a comment - This first set of comments is relative to the structure/build/packaging (ie non-code) issues: I think the jar name should include "recipes" ie for locks: zookeeper-3.2.0-recipes-lock.jar how about a "superset" recipes/zookeeper-3.2.0-recipes.jar with all recipes included? users can choose which to use There seems to be an issue with recipes in release build - actually same issue with contrib: you cannot run "ant test" on lock recipe in the released tar file build using "ant tar" seems build.xml and build-recipes.xml are missing from tar file docs - I think it's ok not to have specific forrest docs inside locsk, but need: recipes/README.txt what is this code? what are requirements for authors? (like interop btw c/java for particular impl) also req that recipe be documented in recipes documentation in forrest describe and point to forrest docs in docs/... (should put link to live h.a.o doc as well) is there some high level bit of informatin that you want to convey to authors re how components should be designed (in particular session mgmt, error handling, logging, etc...) this is a good place to put recipes/lock/README.txt describe and point to forrest docs in docs/... (should put link to live h.a.o doc as well) pointer to javadoc (ie live h.a.o site)? are the forrest recipe docs up to date relative to impl for locks? which lock in particular from docs. any deviation(s), any specific impl detail that's important, configurability? might be good to update the forrest recipe docs to point to this jar/code for implementation ie let users know that they can get this from the release in recipes/locks (they don't need to impl) how do we verify interop? great job adding tests! but they seem to be tests for c, tests for java, but not interop. this seems pretty important. We want to document in recipes/readme that authors in particular should code for c/java to interop for any one particular recipe instance. also that recipes can co-exists in user space (ie placement of nodes, etc...) Might be good enough to add a new JIRA for this, but need to impl this asap. You've done a great job, but I'm holding this jira to a higher standard since ppl who implement further recipes will use your lock implementation as a "blueprint".
          Hide
          Mahadev konar added a comment -

          i attached header files to package.html
          makefile.am
          configure.ac

          adn the others have diffierenct headers which are not apache but can be included in the codebase.

          Show
          Mahadev konar added a comment - i attached header files to package.html makefile.am configure.ac adn the others have diffierenct headers which are not apache but can be included in the codebase.
          Hide
          Patrick Hunt added a comment -

          These comments are relative to the java code:

          cleanup imports

          writelocktest indentation problem line 87

          do we want to talk about leaderelection in locks, or just implement another le in recipes (small wrapper)?

          znodename.java - log failure in exception handlers

          protocolsupport retrydelay eats interrupt, ie lock cannot be interrupted

          shouldn't writelock.lock throw connectionloss if closed?
          the javadoc is not clear - what happens in the "later" case? does this block? what happens if expired or discon? etc...

          line 148 of writelock
          long sessionId = zookeeper.getSessionId();
          what ensures that sessionid you get back is valid? (ie non-zero)
          This is pretty large anony class - why not have a specific static class definition?
          Plus all in one method rather than broken out (even indentation is deep)
          Would be easier to maintain if broken into several methods

          writelock line 208
          try

          { lock(); }

          catch (Exception e)

          { LOG.warn("Failed to acquire lock: " + e, e); }

          what happens if lock returns false (isclosed for example)?

          Some of these classes look reusable (prot/name/op) - should they be in this recipe or elsewhere?
          If we want to enable additional recipes to be easily added based on the work you've done (blueprint)
          we should try to make these easily reusable.

          Show
          Patrick Hunt added a comment - These comments are relative to the java code: cleanup imports writelocktest indentation problem line 87 do we want to talk about leaderelection in locks, or just implement another le in recipes (small wrapper)? znodename.java - log failure in exception handlers protocolsupport retrydelay eats interrupt, ie lock cannot be interrupted shouldn't writelock.lock throw connectionloss if closed? the javadoc is not clear - what happens in the "later" case? does this block? what happens if expired or discon? etc... line 148 of writelock long sessionId = zookeeper.getSessionId(); what ensures that sessionid you get back is valid? (ie non-zero) This is pretty large anony class - why not have a specific static class definition? Plus all in one method rather than broken out (even indentation is deep) Would be easier to maintain if broken into several methods writelock line 208 try { lock(); } catch (Exception e) { LOG.warn("Failed to acquire lock: " + e, e); } what happens if lock returns false (isclosed for example)? Some of these classes look reusable (prot/name/op) - should they be in this recipe or elsewhere? If we want to enable additional recipes to be easily added based on the work you've done (blueprint) we should try to make these easily reusable.
          Hide
          Patrick Hunt added a comment -

          comments on the c code:

          the api methods are named with prefix zoo_mutex_*
          seems to me it would be better to name them zoo_recipe_lock_* as prefix
          this specifies that it's a recipe, in particular that it's implementing the "lock" recipe
          We would expect (and should document in src/recipe/readme) that c api methods
          should be named in this fashion, ie zoo_recipe_<recipename>_*
          otw the c global namespace is going to cause problems down the line
          also easier for users to identify the recipe to which a particular method call pertains

          it's good to commit the configure scripts (see hadoop c code for which files)
          this was lesson learned for cbinding & zkfuse, makes it easier for users to build
          ensure binary flag set in tar package for executable scripts

          would be nice to have a readme to orient the new user in src/c
          include how to build (easier if configure script is here)

          out of the box the patch fails to run-check
          Running Zookeeper_locktest::testlocksh: ./tests/zkServer.sh: Permission denied
          sh: ./tests/zkServer.sh: Permission denied
          : assertion : assertion
          be sure to set the exec flag when committing zkServer to svn.

          in tests might want to use localhost rather than 127.0.0.1 (if ipv6 only host? or does ipv6 handle that?)

          sleep(30) in test is unfortunate - it artificially inflates the test time. having short test time is a win
          any chance you can actively poll rather than just sleeping? something reusable by other recipes would be grt

          I don't see any logging in zoo_lock.c, I think we should log at least the error conditions, also things like
          loop retry at debug level. also operation is pretty complex, some logging in there would help.

          also some similar comments as java - for example getting client_id in operation, this may be invalid
          if the session is not established?

          the api says:

          • \return the return code.
            for many of the methods. what does this mean?
          Show
          Patrick Hunt added a comment - comments on the c code: the api methods are named with prefix zoo_mutex_* seems to me it would be better to name them zoo_recipe_lock_* as prefix this specifies that it's a recipe, in particular that it's implementing the "lock" recipe We would expect (and should document in src/recipe/readme) that c api methods should be named in this fashion, ie zoo_recipe_<recipename>_* otw the c global namespace is going to cause problems down the line also easier for users to identify the recipe to which a particular method call pertains it's good to commit the configure scripts (see hadoop c code for which files) this was lesson learned for cbinding & zkfuse, makes it easier for users to build ensure binary flag set in tar package for executable scripts would be nice to have a readme to orient the new user in src/c include how to build (easier if configure script is here) out of the box the patch fails to run-check Running Zookeeper_locktest::testlocksh: ./tests/zkServer.sh: Permission denied sh: ./tests/zkServer.sh: Permission denied : assertion : assertion be sure to set the exec flag when committing zkServer to svn. in tests might want to use localhost rather than 127.0.0.1 (if ipv6 only host? or does ipv6 handle that?) sleep(30) in test is unfortunate - it artificially inflates the test time. having short test time is a win any chance you can actively poll rather than just sleeping? something reusable by other recipes would be grt I don't see any logging in zoo_lock.c, I think we should log at least the error conditions, also things like loop retry at debug level. also operation is pretty complex, some logging in there would help. also some similar comments as java - for example getting client_id in operation, this may be invalid if the session is not established? the api says: \return the return code. for many of the methods. what does this mean?
          Hide
          Patrick Hunt added a comment -

          running doxygen-doc doesn't result in any c-docs being generated (html).

          Also the version is wrong in .ac file - should be 3.2.0 not 2.2.0
          which reminds me - yet another place we need ot update during a release... we should
          really try to figure out how to centralize this (perhaps Giri knows?) otw it's an increasing
          pain/error point during releases. (not this jira responsible though)

          Show
          Patrick Hunt added a comment - running doxygen-doc doesn't result in any c-docs being generated (html). Also the version is wrong in .ac file - should be 3.2.0 not 2.2.0 which reminds me - yet another place we need ot update during a release... we should really try to figure out how to centralize this (perhaps Giri knows?) otw it's an increasing pain/error point during releases. (not this jira responsible though)
          Hide
          Benjamin Reed added a comment -

          wow nice work. it's nice to have someone that knows automake again!

          some comments:

          in unlock, we shouldn't call the callback if the unlock had an error should we?

          in zope.execute() how would id be null in the 2nd if block?

          Collections.sort() would simplify your sorting wouldn't it?

          if exists() fails when trying to grab the lock, you need to get a new child list and retry.

          for both C and java, shouldn't the lock and unlock be synchronized?

          in zoo_mutex_lock you always nano_sleep. wouldn't it be better to only nano_sleep if you have to retry?

          i think the logic for checking for a previous create with getChildren and create should be implemented the same in C and java.

          with regard to that last comment: currently we do a getChildren() see if a previous create suceeded and then do a create(). one problem with this is that a previously issued create may still be in flight. i think it would be better and simpler to keep doing create until you succeed; do a getChildren(); delete any duplicates.

          Show
          Benjamin Reed added a comment - wow nice work. it's nice to have someone that knows automake again! some comments: in unlock, we shouldn't call the callback if the unlock had an error should we? in zope.execute() how would id be null in the 2nd if block? Collections.sort() would simplify your sorting wouldn't it? if exists() fails when trying to grab the lock, you need to get a new child list and retry. for both C and java, shouldn't the lock and unlock be synchronized? in zoo_mutex_lock you always nano_sleep. wouldn't it be better to only nano_sleep if you have to retry? i think the logic for checking for a previous create with getChildren and create should be implemented the same in C and java. with regard to that last comment: currently we do a getChildren() see if a previous create suceeded and then do a create(). one problem with this is that a previously issued create may still be in flight. i think it would be better and simpler to keep doing create until you succeed; do a getChildren(); delete any duplicates.
          Hide
          Mahadev konar added a comment - - edited

          thanks for looking at it ben and pat... i am fixing most of the stuff you guys mentioned except for the following –

          • the zookeeper c client methods -
            pat's idea does make sense for namespace conventions. But zoo_recipes_lock_methodname seems too long. I recommend we use zoo_recipename_methodname().. which is a little mroe convenient thatn the older one.

          there are others I will prefer opening a jira later to fix it.. (like interperability of c and java code).. i would like this patch to get committed as soon as possible. maintaing such a huge patch is a lot of work...

          Show
          Mahadev konar added a comment - - edited thanks for looking at it ben and pat... i am fixing most of the stuff you guys mentioned except for the following – the zookeeper c client methods - pat's idea does make sense for namespace conventions. But zoo_recipes_lock_methodname seems too long. I recommend we use zoo_recipename_methodname().. which is a little mroe convenient thatn the older one. there are others I will prefer opening a jira later to fix it.. (like interperability of c and java code).. i would like this patch to get committed as soon as possible. maintaing such a huge patch is a lot of work...
          Hide
          Mahadev konar added a comment -

          this patch should fix most of the issues raised by pat and ben.
          you will have to do the following before you can apply the patch –

          svn move src/c/src/zk_log.h src/c/include/zk_log.h 
          

          the include file is necessary to use LOG_* macros.

          ill just list out what I have fixed in this patch..

          • improved the docs for recipes amd recipes/lock. both of them have a README.txt. the recipes one lists out how to create new recipes and conventions to follow.
          • changed the c libary method names to follow the convention
          • renamed the jar file to be zookeeper-version-recipes-name.jar
          • for interop I will open another jira
          • also we should commit the configure generated after running autoconf. I didnt attach it to the patch since it makes it huge.
          • cleaned up imports
          • modularized the java code
          • i have left the sleep 30 in the c tests. I will open a jira to fix it later.
          • i have added logging to the c code.
          • improved the javadocs and c doxygen comments to make the return values clearer
          • chanegd the version in configure.ac
          • the doxygen docs should work now
          • added synchronized for the lock methods both in c and java
          • fixed to sleep only on a retyr

          I think i might have addressed most of the review issues, but please take a look and see if anything still remains. comments are welcome.

          Show
          Mahadev konar added a comment - this patch should fix most of the issues raised by pat and ben. you will have to do the following before you can apply the patch – svn move src/c/src/zk_log.h src/c/include/zk_log.h the include file is necessary to use LOG_* macros. ill just list out what I have fixed in this patch.. improved the docs for recipes amd recipes/lock. both of them have a README.txt. the recipes one lists out how to create new recipes and conventions to follow. changed the c libary method names to follow the convention renamed the jar file to be zookeeper-version-recipes-name.jar for interop I will open another jira also we should commit the configure generated after running autoconf. I didnt attach it to the patch since it makes it huge. cleaned up imports modularized the java code i have left the sleep 30 in the c tests. I will open a jira to fix it later. i have added logging to the c code. improved the javadocs and c doxygen comments to make the return values clearer chanegd the version in configure.ac the doxygen docs should work now added synchronized for the lock methods both in c and java fixed to sleep only on a retyr I think i might have addressed most of the review issues, but please take a look and see if anything still remains. comments are welcome.
          Hide
          Patrick Hunt added a comment -

          zk_log.h is moved, however Makefile.am has not been updated.
          be sure to include it in pkginclude_HEADERS

          I'd rather that the include name be "zookeeper_log.h" to be consistent with with the existing files.
          (granted some of the files are mis-named, like recordio.h, but we should fix that in 4.0)

          Show
          Patrick Hunt added a comment - zk_log.h is moved, however Makefile.am has not been updated. be sure to include it in pkginclude_HEADERS I'd rather that the include name be "zookeeper_log.h" to be consistent with with the existing files. (granted some of the files are mis-named, like recordio.h, but we should fix that in 4.0)
          Hide
          Mahadev konar added a comment -

          this patch addresses pat's concern.
          also

          • modularized the c code methods a little more so that its more readable
          • changed the return codes and relevant doxygen comments to be in line with c libraries (like 0 for success and not zero for failure).
          • the zk_log.h needs to be moved to src/c/include as I had mentioned earlier.
          Show
          Mahadev konar added a comment - this patch addresses pat's concern. also modularized the c code methods a little more so that its more readable changed the return codes and relevant doxygen comments to be in line with c libraries (like 0 for success and not zero for failure). the zk_log.h needs to be moved to src/c/include as I had mentioned earlier.
          Hide
          Mahadev konar added a comment -

          running through hudson

          Show
          Mahadev konar added a comment - running through hudson
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 39 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 111 release audit warnings (more than the trunk's current 105 warnings).

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/artifact/trunk/current/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/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/12405803/ZOOKEEPER-78.patch against trunk revision 766160. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 39 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 111 release audit warnings (more than the trunk's current 105 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/41/console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          -1 for release audit is expected because some files are checked in with non apache headers (but are compatible with apache).

          Show
          Mahadev konar added a comment - -1 for release audit is expected because some files are checked in with non apache headers (but are compatible with apache).
          Hide
          Flavio Junqueira added a comment -

          +1, it looks good to me.

          Show
          Flavio Junqueira added a comment - +1, it looks good to me.
          Hide
          Patrick Hunt added a comment -

          I'd like to commit this asap - just waiting on the change to name the log file "zookeeper_log.h" when it's
          moved into the public include directory.

          see my comment: https://issues.apache.org/jira/browse/ZOOKEEPER-78?focusedCommentId=12698586&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12698586

          Show
          Patrick Hunt added a comment - I'd like to commit this asap - just waiting on the change to name the log file "zookeeper_log.h" when it's moved into the public include directory. see my comment: https://issues.apache.org/jira/browse/ZOOKEEPER-78?focusedCommentId=12698586&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12698586
          Hide
          Mahadev konar added a comment -

          this patch should the fix the zk_log.h issue.

          thanks for pointing it out pat.

          Show
          Mahadev konar added a comment - this patch should the fix the zk_log.h issue. thanks for pointing it out pat.
          Hide
          Patrick Hunt added a comment -

          Committed revision 768067.

          Thanks Mahadev!

          Show
          Patrick Hunt added a comment - Committed revision 768067. Thanks Mahadev!
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/48/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/12406269/ZOOKEEPER-78.patch against trunk revision 768067. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 42 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/48/console This message is automatically generated.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #289 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/289/)
          . added a high level protocol/feature - for easy Leader Election or exclusive Write Lock creation

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #289 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/289/ ) . added a high level protocol/feature - for easy Leader Election or exclusive Write Lock creation

            People

            • Assignee:
              Mahadev konar
              Reporter:
              james strachan
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development