ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-965

Need a multi-update command to allow multiple znodes to be updated safely

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.3
    • Fix Version/s: 3.4.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The basic idea is to have a single method called "multi" that will accept a list of create, delete, update or check objects each of which has a desired version or file state in the case of create. If all of the version and existence constraints can be satisfied, then all updates will be done atomically.

      Two API styles have been suggested. One has a list as above and the other style has a "Transaction" that allows builder-like methods to build a set of updates and a commit method to finalize the transaction. This can trivially be reduced to the first kind of API so the list based API style should be considered the primitive and the builder style should be implemented as syntactic sugar.

      The total size of all the data in all updates and creates in a single transaction should be limited to 1MB.

      Implementation-wise this capability can be done using standard ZK internals. The changes include:

      • update to ZK clients to all the new call
      • additional wire level request
      • on the server, in the code that converts transactions to idempotent form, the code should be slightly extended to convert a list of operations to idempotent form.
      • on the client, a down-rev server that rejects the multi-update should be detected gracefully and an informative exception should be thrown.

      To facilitate shared development, I have established a github repository at https://github.com/tdunning/zookeeper and am happy to extend committer status to anyone who agrees to donate their code back to Apache. The final patch will be attached to this bug as normal.

      1. ZOOKEEPER-965.patch
        174 kB
        Ted Dunning
      2. ZOOKEEPER-965.patch
        174 kB
        Ted Dunning
      3. ZOOKEEPER-965.patch
        174 kB
        Ted Dunning
      4. ZOOKEEPER-965.patch
        172 kB
        Ted Dunning
      5. ZOOKEEPER-965.patch
        172 kB
        Ted Dunning
      6. ZOOKEEPER-965.patch
        253 kB
        Ted Dunning
      7. ZOOKEEPER-965.patch
        172 kB
        Ted Dunning
      8. ZOOKEEPER-965.patch
        176 kB
        Ted Dunning
      9. ZOOKEEPER-965.patch
        167 kB
        Ted Dunning
      10. ZOOKEEPER-965.patch
        169 kB
        Ted Dunning
      11. ZOOKEEPER-965.patch
        166 kB
        Ted Dunning
      12. ZOOKEEPER-965.patch
        162 kB
        Ted Dunning
      13. ZOOKEEPER-965.patch
        151 kB
        Ted Dunning
      14. ZOOKEEPER-965.patch
        154 kB
        Ted Dunning
      15. ZOOKEEPER-965.patch
        151 kB
        Ted Dunning
      16. ZOOKEEPER-965.patch
        133 kB
        Ted Dunning
      17. ZOOKEEPER-965.patch
        135 kB
        Ted Dunning
      18. ZOOKEEPER-965.patch
        133 kB
        Ted Dunning
      19. ZOOKEEPER-965.patch
        128 kB
        Ted Dunning
      20. ZOOKEEPER-965.patch
        128 kB
        Ted Dunning
      21. ZOOKEEPER-965.patch
        150 kB
        Ted Dunning

        Issue Links

        1.
        Client side for multi Sub-task Closed Unassigned
         
        2.
        Server side decoding and function dispatch Sub-task Closed Unassigned
         
        3.
        Database multi-update Sub-task Closed Unassigned
         

          Activity

          Ted Dunning created issue -
          Hide
          Ted Dunning added a comment -

          I have committed a minimal set of client side changes to the github repo mentioned in the description.

          This includes:

          jute changes to encode a few new things

          a Record class that encodes multiple transactions

          an Op class and sub-classes that encode the necessary operations

          a new multi() method on Zookeeper that send the request to the server (which will still fail to respond).

          Reviews, comments and updates are welcome!

          Show
          Ted Dunning added a comment - I have committed a minimal set of client side changes to the github repo mentioned in the description. This includes: jute changes to encode a few new things a Record class that encodes multiple transactions an Op class and sub-classes that encode the necessary operations a new multi() method on Zookeeper that send the request to the server (which will still fail to respond). Reviews, comments and updates are welcome!
          Thomas Koch made changes -
          Field Original Value New Value
          Link This issue relates to ZOOKEEPER-911 [ ZOOKEEPER-911 ]
          Hide
          Thomas Koch added a comment -

          I think it would help if we'd have the API proposed in ZOOKEEPER-911. So you would have operation objects for the user which could be added to a multi-transaction

          Show
          Thomas Koch added a comment - I think it would help if we'd have the API proposed in ZOOKEEPER-911 . So you would have operation objects for the user which could be added to a multi-transaction
          Hide
          Thomas Koch added a comment -

          Continuing a short conversation from https://github.com/tdunning/zookeeper/commit/1b1cbff2e38bc04b53174381b485304c87d5f743 here.

          We could introduce operation classes for all operations, including getChildren, getData and exists with ZOOKEEPER-911 and use these operation classes instead of the Op classes introduced by Ted. The MultiTransaction class would of course only accept write operations (Create, setData, Delete). This could be enforced either manually or with an interface "WriteOperation".

          Having an interface "Operation" doesn't mean you couldn't have an abstract "BaseOperation" which could implement interface and be a base for the other operations to extend. I also don't like to have interfaces for everything. But making Operation an interface could make testing a lot easier.

          Regarding the naming Op vs. Operation: We have more then 640k of memory nowadays and screens show more then 80 characters in a line. So we gained the possibility to write readable code. If you talk about it, would you say "Op" or "Operation"? So to avoid confusion it makes sense to write the class the same as you'd call it when speaking. Even with modern IDEs there may be cases when one wants to search for names. "Op" could also be in "Option", "Operator", "Open", .

          If you don't mind, I really loved reading "Clean Code" from Robert C. Martin. He also explains naming guidelines and it's a nice book to read.

          Show
          Thomas Koch added a comment - Continuing a short conversation from https://github.com/tdunning/zookeeper/commit/1b1cbff2e38bc04b53174381b485304c87d5f743 here. We could introduce operation classes for all operations, including getChildren, getData and exists with ZOOKEEPER-911 and use these operation classes instead of the Op classes introduced by Ted. The MultiTransaction class would of course only accept write operations (Create, setData, Delete). This could be enforced either manually or with an interface "WriteOperation". Having an interface "Operation" doesn't mean you couldn't have an abstract "BaseOperation" which could implement interface and be a base for the other operations to extend. I also don't like to have interfaces for everything. But making Operation an interface could make testing a lot easier. Regarding the naming Op vs. Operation: We have more then 640k of memory nowadays and screens show more then 80 characters in a line. So we gained the possibility to write readable code. If you talk about it, would you say "Op" or "Operation"? So to avoid confusion it makes sense to write the class the same as you'd call it when speaking. Even with modern IDEs there may be cases when one wants to search for names. "Op" could also be in "Option", "Operator", "Open", . If you don't mind, I really loved reading "Clean Code" from Robert C. Martin. He also explains naming guidelines and it's a nice book to read.
          Hide
          Ted Dunning added a comment -

          I don't like the use of an interface for Op since it really does share function. I can't see how testing would be facilitated by having an interface there.

          Also, Op is of long standard as an abbreviation for Operation (it appears in other Zookeeper code, in multiple dictionaries in the form of op code and in the IBM 650 manual from 1954).

          Regarding 911, I would really like to finish this issue off with a limited mission before taking on other JIRA's in the same patch.

          Show
          Ted Dunning added a comment - I don't like the use of an interface for Op since it really does share function. I can't see how testing would be facilitated by having an interface there. Also, Op is of long standard as an abbreviation for Operation (it appears in other Zookeeper code, in multiple dictionaries in the form of op code and in the IBM 650 manual from 1954). Regarding 911, I would really like to finish this issue off with a limited mission before taking on other JIRA's in the same patch.
          Hide
          Ted Dunning added a comment -

          Added unit tests in the github version.

          Thanks to Thomas who was kind enough to provide several comments on the code.

          Show
          Ted Dunning added a comment - Added unit tests in the github version. Thanks to Thomas who was kind enough to provide several comments on the code.
          Hide
          Thomas Koch added a comment -

          Please Ted, don't move that fast. I've three outstanding issues that needs review or comments from commiters and that could influence your patch. Your current patch directly conflicts with ZOOKEEPER-911 and how I'd propose to implement it. I'm waiting on a comment for ZOOKEEPER-849 (on the mailing list) to continue with that. And once ZOOKEEPER-837 would be committed I could continue with ZOOKEEPER-911.

          I'm also working hard to clean up the mess that ZooKeeper currently still is and would ask you not to make it worse with your patch.

          Things are moving extremely slow here at ZooKeeper because commiters don't have time for reviews or comments. I'm working since may (more then six months now) to slowly move the ZooKeeper API in a usable state.

          I gave a presentation about the internals of ZooKeeper in my team before christmas. People were pretty much scared at the poor code quality of such a central peace of some NoSQL systems.

          Could you also be so kind please and provide a use case for your request? The issue of transactions has been raised multiple times on the mailing list and one standard answer was to rather use one ZNode that'd contain all necessary informations. Maybe there's an easier solution to what you intend to do?

          Hope I don't sound too rude. I feel a little frustrated.

          Show
          Thomas Koch added a comment - Please Ted, don't move that fast. I've three outstanding issues that needs review or comments from commiters and that could influence your patch. Your current patch directly conflicts with ZOOKEEPER-911 and how I'd propose to implement it. I'm waiting on a comment for ZOOKEEPER-849 (on the mailing list) to continue with that. And once ZOOKEEPER-837 would be committed I could continue with ZOOKEEPER-911 . I'm also working hard to clean up the mess that ZooKeeper currently still is and would ask you not to make it worse with your patch. Things are moving extremely slow here at ZooKeeper because commiters don't have time for reviews or comments. I'm working since may (more then six months now) to slowly move the ZooKeeper API in a usable state. I gave a presentation about the internals of ZooKeeper in my team before christmas. People were pretty much scared at the poor code quality of such a central peace of some NoSQL systems. Could you also be so kind please and provide a use case for your request? The issue of transactions has been raised multiple times on the mailing list and one standard answer was to rather use one ZNode that'd contain all necessary informations. Maybe there's an easier solution to what you intend to do? Hope I don't sound too rude. I feel a little frustrated.
          Hide
          Ted Dunning added a comment -

          Please Ted, don't move that fast.

          My time is only available in tiny drips so I have to use them when I get them. You must be the first
          person to have accused me of moving too fast on open source projects.

          I think that you will have a slow time of it changing the ZK API except with an add-on library because of the all the backwards compatibility issues that will raise. I doubt seriously that any fundamental changes will be acceptable to the community.

          As such, I am certainly not going to hold off on this change waiting for an API change that may not happen.

          As far as use cases are concerned, there have been several recently on the mailing list. They have been compelling enough that I have changed my mind about the value of multi-update and would like to contribute to making it happen. One excellent use-case was the graph idea where nodes have connections to and from other nodes. It is unreasonable to keep a relatively large graph in a single node, but it is also important to have coherent updates to the graph. Multi-update makes this trivial.

          Show
          Ted Dunning added a comment - Please Ted, don't move that fast. My time is only available in tiny drips so I have to use them when I get them. You must be the first person to have accused me of moving too fast on open source projects. I think that you will have a slow time of it changing the ZK API except with an add-on library because of the all the backwards compatibility issues that will raise. I doubt seriously that any fundamental changes will be acceptable to the community. As such, I am certainly not going to hold off on this change waiting for an API change that may not happen. As far as use cases are concerned, there have been several recently on the mailing list. They have been compelling enough that I have changed my mind about the value of multi-update and would like to contribute to making it happen. One excellent use-case was the graph idea where nodes have connections to and from other nodes. It is unreasonable to keep a relatively large graph in a single node, but it is also important to have coherent updates to the graph. Multi-update makes this trivial.
          Hide
          Henry Robinson added a comment -

          Hi Ted -

          I took a quick look at the github branch. Looks really good, thanks.

          I've got a few comments on the code itself, but I'll save those until you post a patch. My main issue is the following: the 'multi' API call is expressed in terms of an iterable over a polymorphic type, both of which are Java features that aren't extant in C. To aid future language bindings authors and to make the implementation really easy to verify, I'd like to see an API signature that's very easily translated between languages. The iterable isn't too concerning (almost every language has some notion of lists) but the polymorphic op object should map onto some simpler struct type.

          I know that the serialisation is independent of the signature, so we could call it whatever we liked in any language, but I'd like to keep the core ZK API consistent across all bindings where possible and use wrappers in, for example, Python to provide more idiomatic interfaces. The serialisation may also change when we finally vote jute off the island, so we can't use that as the API spec. Indeed, we'll probably use Avro, where we have to write APIs in language-agnostic IDLs.

          So, to cut a long story short: any chance you can make the API a bit more language neutral? Then the op stuff can be a (very) thin wrapper. Shouldn't be a large change at all.

          You might consider chopping this up into a few JIRAs (apologies if you have and I've missed them) - core API, Java wrapper, finishing touches (like payload size limits).

          Excited to see this! Let me know how I can help.

          Henry

          Show
          Henry Robinson added a comment - Hi Ted - I took a quick look at the github branch. Looks really good, thanks. I've got a few comments on the code itself, but I'll save those until you post a patch. My main issue is the following: the 'multi' API call is expressed in terms of an iterable over a polymorphic type, both of which are Java features that aren't extant in C. To aid future language bindings authors and to make the implementation really easy to verify, I'd like to see an API signature that's very easily translated between languages. The iterable isn't too concerning (almost every language has some notion of lists) but the polymorphic op object should map onto some simpler struct type. I know that the serialisation is independent of the signature, so we could call it whatever we liked in any language, but I'd like to keep the core ZK API consistent across all bindings where possible and use wrappers in, for example, Python to provide more idiomatic interfaces. The serialisation may also change when we finally vote jute off the island, so we can't use that as the API spec. Indeed, we'll probably use Avro, where we have to write APIs in language-agnostic IDLs. So, to cut a long story short: any chance you can make the API a bit more language neutral? Then the op stuff can be a (very) thin wrapper. Shouldn't be a large change at all. You might consider chopping this up into a few JIRAs (apologies if you have and I've missed them) - core API, Java wrapper, finishing touches (like payload size limits). Excited to see this! Let me know how I can help. Henry
          Hide
          Henry Robinson added a comment -

          Hi Thomas -

          I really appreciate all your hard work cleaning up ZooKeeper's internals. I understand your frustration about the speed at which some tickets are moving. You've correctly identified that the committers have limited time, and particularly so over the holiday season. Hopefully we can pick up the pace now!

          However, I'm uncomfortable with the idea that ongoing refactoring work could block an often asked-for feature like this - particularly a JIRA (911) where there isn't yet consensus on the approach, or indeed an available patch. Open source projects see fluctuating participation so we can't afford generally for issues to come with 'locks' on the code they touch, otherwise we run the risk of starvation

          So if this issue gets a patch with consensus before ZOOKEEPER-911, I'll be very happy to commit it and then to work with you on the extra changes in ZOOKEEPER-911 that this patch would cause.

          Henry

          Show
          Henry Robinson added a comment - Hi Thomas - I really appreciate all your hard work cleaning up ZooKeeper's internals. I understand your frustration about the speed at which some tickets are moving. You've correctly identified that the committers have limited time, and particularly so over the holiday season. Hopefully we can pick up the pace now! However, I'm uncomfortable with the idea that ongoing refactoring work could block an often asked-for feature like this - particularly a JIRA (911) where there isn't yet consensus on the approach, or indeed an available patch. Open source projects see fluctuating participation so we can't afford generally for issues to come with 'locks' on the code they touch, otherwise we run the risk of starvation So if this issue gets a patch with consensus before ZOOKEEPER-911 , I'll be very happy to commit it and then to work with you on the extra changes in ZOOKEEPER-911 that this patch would cause. Henry
          Hide
          Henry Robinson added a comment -

          Assigning to Ted.

          Show
          Henry Robinson added a comment - Assigning to Ted.
          Henry Robinson made changes -
          Assignee Ted Dunning [ tdunning ]
          Hide
          Ted Dunning added a comment -

          Henry,

          I can definitely set up some sub-tasks.

          Relative to the iterable and polymorphism, I don't much see the problem. In particular, I think I set it up so that the API visible components are independent of the lower levels.

          In C, for instance, using a list/array/whatever of structs with a type field and a super set of all possible fields would work out just about as easily as the Java version. The jute objects don't overlap and both Java and C api's would have to be creating and serializing the jute objects. When jute gets the boot, I would presume that we would have something similar in terms of a separation between the API level structures and the serialization level structures. In particular, I simply re-used most of the request structures in the multi request and only added the single check structure (and the multi for the overall header).

          So I really don't see the problem on the C side of things. To be concrete, I see that the C side could have something kind of like:

          result multi(op[] ops) // assumes last op is null

          or

          result multi(op[] ops, int opCount)

          where

          {verbatim}
          typedef struct { int type; // always present char *path; // always present uint8 * data; // only with create, setData ACL* acl; // only for create int version; // only for delete, setData }{verbatim}

          Doesn't this match up well enough with the Java side of the house? I would be loathe to lose the idiomatic Java idiom.

          Show
          Ted Dunning added a comment - Henry, I can definitely set up some sub-tasks. Relative to the iterable and polymorphism, I don't much see the problem. In particular, I think I set it up so that the API visible components are independent of the lower levels. In C, for instance, using a list/array/whatever of structs with a type field and a super set of all possible fields would work out just about as easily as the Java version. The jute objects don't overlap and both Java and C api's would have to be creating and serializing the jute objects. When jute gets the boot, I would presume that we would have something similar in terms of a separation between the API level structures and the serialization level structures. In particular, I simply re-used most of the request structures in the multi request and only added the single check structure (and the multi for the overall header). So I really don't see the problem on the C side of things. To be concrete, I see that the C side could have something kind of like: result multi(op[] ops) // assumes last op is null or result multi(op[] ops, int opCount) where {verbatim} typedef struct { int type; // always present char *path; // always present uint8 * data; // only with create, setData ACL* acl; // only for create int version; // only for delete, setData }{verbatim} Doesn't this match up well enough with the Java side of the house? I would be loathe to lose the idiomatic Java idiom.
          Hide
          Thomas Koch added a comment -

          Hi,

          I uploaded a patch for ZOOKEEPER-911. Please have a look how you could use the provided Operation classes in ZK911.

          Show
          Thomas Koch added a comment - Hi, I uploaded a patch for ZOOKEEPER-911 . Please have a look how you could use the provided Operation classes in ZK911.
          Hide
          Henry Robinson added a comment -

          Hi Ted -

          You don't have to lose the idiomatic Java - I like the API. I'm just distinguishing between the API provided by the Java client and the API that the Java client calls in ZooKeeper, which should not be idiomatic. Currently that API is defined by the serialisation (and this is true for most API calls, not just multi(...)) rather than some abstract API signature which is realisable in every implementing language - I want to make sure that the serialisation is not the specification. Again, an Avro or Thrift or whatever IDL API would make these issues go away.

          However, the idiomatic changes here are so slight that having thought about it overnight I'm not too concerned about separating it out. It would be different if, e.g. the API was heavily object-oriented (for example a builder interface); then I would mandate that such an API should wrap a simple procedural API. It's pretty clear here what the multi(...) API means in all languages we're interested in.

          Thanks for listening

          Henry

          Show
          Henry Robinson added a comment - Hi Ted - You don't have to lose the idiomatic Java - I like the API. I'm just distinguishing between the API provided by the Java client and the API that the Java client calls in ZooKeeper, which should not be idiomatic. Currently that API is defined by the serialisation (and this is true for most API calls, not just multi(...)) rather than some abstract API signature which is realisable in every implementing language - I want to make sure that the serialisation is not the specification. Again, an Avro or Thrift or whatever IDL API would make these issues go away. However, the idiomatic changes here are so slight that having thought about it overnight I'm not too concerned about separating it out. It would be different if, e.g. the API was heavily object-oriented (for example a builder interface); then I would mandate that such an API should wrap a simple procedural API. It's pretty clear here what the multi(...) API means in all languages we're interested in. Thanks for listening Henry
          Hide
          Ted Dunning added a comment -

          However, the idiomatic changes here are so slight that having thought about it overnight I'm not too concerned about separating it out. It would be different if, e.g. the API was heavily object-oriented (for example a builder interface); then I would mandate that such an API should wrap a simple procedural API. It's pretty clear here what the multi(...) API means in all languages we're interested in.

          For what it is worth, I plan to throw in a builder API as well. It will, however, be naught more than syntactic cover over the API you see already.

          Show
          Ted Dunning added a comment - However, the idiomatic changes here are so slight that having thought about it overnight I'm not too concerned about separating it out. It would be different if, e.g. the API was heavily object-oriented (for example a builder interface); then I would mandate that such an API should wrap a simple procedural API. It's pretty clear here what the multi(...) API means in all languages we're interested in. For what it is worth, I plan to throw in a builder API as well. It will, however, be naught more than syntactic cover over the API you see already.
          Hide
          Vishal Kher added a comment -

          Hi Ted,

          Will this induce any change to watches (especially when one transactions is modifying same znode multiple times)?

          Thanks.

          Show
          Vishal Kher added a comment - Hi Ted, Will this induce any change to watches (especially when one transactions is modifying same znode multiple times)? Thanks.
          Hide
          Ted Dunning added a comment -

          I would anticipate no changes to the way that watches work. All that is happening here is that a group of transactions will all be applied or will all fail. These operations could have been done separately with the same effect insofar as watches are concerned.

          Remember that you may only get a single watch notification for several updates even without the multi command.

          Sent from my iPhone

          Show
          Ted Dunning added a comment - I would anticipate no changes to the way that watches work. All that is happening here is that a group of transactions will all be applied or will all fail. These operations could have been done separately with the same effect insofar as watches are concerned. Remember that you may only get a single watch notification for several updates even without the multi command. Sent from my iPhone
          Hide
          Jared Cantwell added a comment -

          Hey Ted,

          Has there been any progress on this issue? We would really love to see this happen, so just pinging to see what's left....

          Show
          Jared Cantwell added a comment - Hey Ted, Has there been any progress on this issue? We would really love to see this happen, so just pinging to see what's left....
          Hide
          Ted Dunning added a comment -

          There has only been limited progress.

          I have the client and server command parsing, wire formats and some message passing defined.

          I have talked to Ben and Mahadev about where the final commit needs to be placed, but not done any code at all.

          My own extra-curricular schedule is completely under water from day job activities. I am happy to help explain what has happened so far, but I am unable to drive further right now.

          I can update my git branch on request to make it easier to see what is going on relative to trunk.

          Show
          Ted Dunning added a comment - There has only been limited progress. I have the client and server command parsing, wire formats and some message passing defined. I have talked to Ben and Mahadev about where the final commit needs to be placed, but not done any code at all. My own extra-curricular schedule is completely under water from day job activities. I am happy to help explain what has happened so far, but I am unable to drive further right now. I can update my git branch on request to make it easier to see what is going on relative to trunk.
          Hide
          Marshall McMullen added a comment -

          Ted,

          I work with Jared and this is becoming an increasingly important issue for us. I have some bandwidth to work on this issue and would like to get started right away. I will of course be donating all my code back to Apache.

          When you have time, could you update your git branch?

          Many thanks!

          Show
          Marshall McMullen added a comment - Ted, I work with Jared and this is becoming an increasingly important issue for us. I have some bandwidth to work on this issue and would like to get started right away. I will of course be donating all my code back to Apache. When you have time, could you update your git branch? Many thanks!
          Hide
          Ted Dunning added a comment -

          I would love to.

          I would also love to schedule a phone (skype) call among all interested parties to make sure that things can go forward as quickly as possible.

          Send email on the list so we can set up a good time. I will be happy to coordinate the call and summarize back to the mailing list.

          Show
          Ted Dunning added a comment - I would love to. I would also love to schedule a phone (skype) call among all interested parties to make sure that things can go forward as quickly as possible. Send email on the list so we can set up a good time. I will be happy to coordinate the call and summarize back to the mailing list.
          Hide
          Ted Dunning added a comment -

          On Wed, Apr 13, 2011 at 2:04 PM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - On Wed, Apr 13, 2011 at 2:04 PM, Marshall McMullen (JIRA)
          Hide
          Ted Dunning added a comment -

          OK. I have rebased to follow trunk and pushed the changes to github.

          See https://github.com/tdunning/zookeeper/tree/zookeeper-966

          Show
          Ted Dunning added a comment - OK. I have rebased to follow trunk and pushed the changes to github. See https://github.com/tdunning/zookeeper/tree/zookeeper-966
          Hide
          Marshall McMullen added a comment -

          Ted, I totally agree, let's set up a skype call for everyone to discuss this and get it moving forward. I'm available anytime during the day to talk, or I can make myself available in the evening/night if that's easier.

          Show
          Marshall McMullen added a comment - Ted, I totally agree, let's set up a skype call for everyone to discuss this and get it moving forward. I'm available anytime during the day to talk, or I can make myself available in the evening/night if that's easier.
          Hide
          Marshall McMullen added a comment -

          By the way, I'm in Colorado (time zone is Mountain) in case that affects when you'd like to schedule the call. I'm happy to do it whenever it's convenient for you though.

          Show
          Marshall McMullen added a comment - By the way, I'm in Colorado (time zone is Mountain) in case that affects when you'd like to schedule the call. I'm happy to do it whenever it's convenient for you though.
          Hide
          Ted Dunning added a comment -

          I sent out a call for the discussion on the zookeeper-dev mailing list.

          The tentative call time is Friday 4/14 at 4PM PDT.

          Please respond to me via direct email at tdunning@apache.org so I can handle the logistics of the call.

          Show
          Ted Dunning added a comment - I sent out a call for the discussion on the zookeeper-dev mailing list. The tentative call time is Friday 4/14 at 4PM PDT. Please respond to me via direct email at tdunning@apache.org so I can handle the logistics of the call.
          Hide
          Marshall McMullen added a comment -

          Ted, I've sent you an email so we can coordinate.

          Date/time works fine for me.

          Thanks.

          Show
          Marshall McMullen added a comment - Ted, I've sent you an email so we can coordinate. Date/time works fine for me. Thanks.
          Hide
          Marshall McMullen added a comment -

          I've been having email discussions with various folks that have been super helpful in getting this implemented. I'm going to try to summaries some of the key points here for posterity and other interested parties.

          Ted's initial version of this adds a new MultiTransactionRecord with an OpCode of 'multi'. This contains a list of all the ops that need to get committed. The design is such that all of the ops must succeed in order for the multi op to succeed. If any one of the ops fails, then the entire multi op must fail.

          The basic approach I've taken to enhance Ted's version is to look at all the processors and enhance them as needed in order to support the new multi op. Here's the flow of the processors (as documented in LeaderZooKeeperServer.java):

          processors: PrepRequestProcessor -> ProposalRequestProcessor ->

          • CommitProcessor -> Leader.ToBeAppliedRequestProcessor ->
          • FinalRequestProcessor

          The purpose of PrepRequestProcessor (pRequest) is essentially to validate the incoming request and ensure (for example) the path we want to create doesn't already exist, or the path we want to delete does exist. So I've enhanced PrepRequestProcessor and added a case statement for 'multi' that essentially iterates over all the ops contained in the multi op and calls pRequest on each. If any of them fails, then it fails. Otherwise, a new MultiTxn will be created. This is *one* transaction that contains all the ops in the multi op.

          Show
          Marshall McMullen added a comment - I've been having email discussions with various folks that have been super helpful in getting this implemented. I'm going to try to summaries some of the key points here for posterity and other interested parties. Ted's initial version of this adds a new MultiTransactionRecord with an OpCode of 'multi'. This contains a list of all the ops that need to get committed. The design is such that all of the ops must succeed in order for the multi op to succeed. If any one of the ops fails, then the entire multi op must fail. The basic approach I've taken to enhance Ted's version is to look at all the processors and enhance them as needed in order to support the new multi op. Here's the flow of the processors (as documented in LeaderZooKeeperServer.java): processors: PrepRequestProcessor -> ProposalRequestProcessor -> CommitProcessor -> Leader.ToBeAppliedRequestProcessor -> FinalRequestProcessor The purpose of PrepRequestProcessor (pRequest) is essentially to validate the incoming request and ensure (for example) the path we want to create doesn't already exist, or the path we want to delete does exist. So I've enhanced PrepRequestProcessor and added a case statement for 'multi' that essentially iterates over all the ops contained in the multi op and calls pRequest on each. If any of them fails, then it fails. Otherwise, a new MultiTxn will be created. This is * one * transaction that contains all the ops in the multi op.
          Hide
          Marshall McMullen added a comment -

          Another key design point we discussed in email was with regard to the order of the ops inside a multi op and the final state that should result. We discussed the notion that the order of the ops inside a multi op must be significant since there is nothing to be gained by having order be non-deterministic. Supposing we have a multi that says:

          update "foo" to 1
          update "foo" to 2

          The end result should be 2.

          Show
          Marshall McMullen added a comment - Another key design point we discussed in email was with regard to the order of the ops inside a multi op and the final state that should result. We discussed the notion that the order of the ops inside a multi op must be significant since there is nothing to be gained by having order be non-deterministic. Supposing we have a multi that says: update "foo" to 1 update "foo" to 2 The end result should be 2.
          Hide
          Camille Fournier added a comment -

          I am out of the office until tuesday april 26. For ServiceBase or GSDiscovery issues please contact gs-servicebase-dev. For Permit questions contact gs-permit-dev.

          Show
          Camille Fournier added a comment - I am out of the office until tuesday april 26. For ServiceBase or GSDiscovery issues please contact gs-servicebase-dev. For Permit questions contact gs-permit-dev.
          Hide
          Marshall McMullen added a comment -

          I've finished the necessary work inside PrepRequestProcessor.java to deal with multi-ops. I'm going to next look at SyncRequestProcessor and then FinalRequestProcessor.

          Show
          Marshall McMullen added a comment - I've finished the necessary work inside PrepRequestProcessor.java to deal with multi-ops. I'm going to next look at SyncRequestProcessor and then FinalRequestProcessor.
          Hide
          Marshall McMullen added a comment -

          I've realized there's a hole in my current implementation. I need to create a MultiTxn.java to exist alongside CreateTxn.java, DeleteTxn.java, etc.

          I wasn't sure how this gets generated, but Mahadev pointed me in the right direction. He indicated I should look at src/zookeeper.jute to create the necessary auto generated file.

          Show
          Marshall McMullen added a comment - I've realized there's a hole in my current implementation. I need to create a MultiTxn.java to exist alongside CreateTxn.java, DeleteTxn.java, etc. I wasn't sure how this gets generated, but Mahadev pointed me in the right direction. He indicated I should look at src/zookeeper.jute to create the necessary auto generated file.
          Hide
          Marshall McMullen added a comment -

          So I've added a new 'class' entry to src/zookeeper.jute for a MultiTxn. But I can't seem to get it to build and generate the new file. I'm still not sure what I want in this MutliTxn, but for now I'd like to get it to build and generate the required java file so I can start using it. Is there something special I need to do other than just running 'ant compile' to generate the files?

          Show
          Marshall McMullen added a comment - So I've added a new 'class' entry to src/zookeeper.jute for a MultiTxn. But I can't seem to get it to build and generate the new file. I'm still not sure what I want in this MutliTxn, but for now I'd like to get it to build and generate the required java file so I can start using it. Is there something special I need to do other than just running 'ant compile' to generate the files?
          Hide
          Marshall McMullen added a comment -

          Never mind, figured it out == "ant compile_jute"

          Show
          Marshall McMullen added a comment - Never mind, figured it out == "ant compile_jute"
          Hide
          Marshall McMullen added a comment -

          Status Update::

          I've made all the changes necessary to PrepRequestProcessor, SyncRequestProcessor, FinalRequestProcessor, DataTree, as well as many, many other classes. I've now gotten all the unit tests that Ted originally wrote to pass. I then spent all day today writing a ton of new unit tests to test strange corner cases, conflicts, and other more complex issues. These identified a few bugs in my implementation which I've since fixed.

          The two remaining tasks I have on my plate that I intend to finish this weekend:
          (1) Finish up the remaining client-side code
          (2) Clean up all the code I've added, document things, etc.

          This weekend I'll commit all my changes to Ted's multi branch.

          Thanks everyone for your help!!

          Show
          Marshall McMullen added a comment - Status Update:: I've made all the changes necessary to PrepRequestProcessor, SyncRequestProcessor, FinalRequestProcessor, DataTree, as well as many, many other classes. I've now gotten all the unit tests that Ted originally wrote to pass. I then spent all day today writing a ton of new unit tests to test strange corner cases, conflicts, and other more complex issues. These identified a few bugs in my implementation which I've since fixed. The two remaining tasks I have on my plate that I intend to finish this weekend: (1) Finish up the remaining client-side code (2) Clean up all the code I've added, document things, etc. This weekend I'll commit all my changes to Ted's multi branch. Thanks everyone for your help!!
          Hide
          Camille Fournier added a comment -

          Awesome Marshall. Please post a patch here when you feel good about it. The sooner people can start reviewing it, the faster it'll get approval to go into trunk.

          Show
          Camille Fournier added a comment - Awesome Marshall. Please post a patch here when you feel good about it. The sooner people can start reviewing it, the faster it'll get approval to go into trunk.
          Hide
          Ted Dunning added a comment -

          yowza Marshall!

          You are code monster. This is really exciting.

          Commit at will. Don't worry about half baked stuff. Getting eyes on semicolons sooner is good.

          Show
          Ted Dunning added a comment - yowza Marshall! You are code monster. This is really exciting. Commit at will. Don't worry about half baked stuff. Getting eyes on semicolons sooner is good.
          Hide
          Marshall McMullen added a comment -

          Thanks everyone for all the help and the kind words. Hopefully when I get this all committed I've done most of it correctly .

          I'm going to go ahead and commit my pending changes to Ted's private git branch. It's not perfect as there are a few things I wanted to work out better but it wasn't working out as well as I'd have liked. Specifically:

          1. No client work is done. I'm not sure where to start on this, so if someone wants to point me the right direction I'll tackle that next.

          2. PrepRequestProcessor.java:432: I don't like how I had to serialize the individual Txns into the MultiTxn only to deserialize it in the next request processor in DataTree. I wanted to just have MultiTxn contain a list of Txns. However, the jute auto code generation stuff got in my way. I couldn't figure out how to make the MultiTxn contain a list of Txns so I made it a buffer that I serialize into/out of. It works, but I think this could be better.

          3. Several places where we do a switch on types of ops inside multi op, and in default it throws an IOException. That
          doesn't seem very appropriate. Maybe instead a KeeperException.APIERROR or perhaps we need a (new) specific exception?

          4. There is currently no way to tell which op in a multi op failed. It's implemented so that it throws the KeeperException corresponding to the first op in the multi op that caused the multi op to pass. However, it might be more useful if there was a way to know exactly which op failed. I tried several approaches to implement this but couldn't get it working correctly. Maybe someone can offer some advice on this.

          I'll commit my pending changes momentarily.

          Show
          Marshall McMullen added a comment - Thanks everyone for all the help and the kind words. Hopefully when I get this all committed I've done most of it correctly . I'm going to go ahead and commit my pending changes to Ted's private git branch. It's not perfect as there are a few things I wanted to work out better but it wasn't working out as well as I'd have liked. Specifically: 1. No client work is done. I'm not sure where to start on this, so if someone wants to point me the right direction I'll tackle that next. 2. PrepRequestProcessor.java:432: I don't like how I had to serialize the individual Txns into the MultiTxn only to deserialize it in the next request processor in DataTree. I wanted to just have MultiTxn contain a list of Txns. However, the jute auto code generation stuff got in my way. I couldn't figure out how to make the MultiTxn contain a list of Txns so I made it a buffer that I serialize into/out of. It works, but I think this could be better. 3. Several places where we do a switch on types of ops inside multi op, and in default it throws an IOException. That doesn't seem very appropriate. Maybe instead a KeeperException.APIERROR or perhaps we need a (new) specific exception? 4. There is currently no way to tell which op in a multi op failed. It's implemented so that it throws the KeeperException corresponding to the first op in the multi op that caused the multi op to pass. However, it might be more useful if there was a way to know exactly which op failed. I tried several approaches to implement this but couldn't get it working correctly. Maybe someone can offer some advice on this. I'll commit my pending changes momentarily.
          Hide
          Marshall McMullen added a comment -

          Alright, got my changes pushed to Ted's multi branch. I'd appreciate feedback/thoughts/suggestions/ideas from anyone who has the time to look these changes over. Thanks so much!

          Show
          Marshall McMullen added a comment - Alright, got my changes pushed to Ted's multi branch. I'd appreciate feedback/thoughts/suggestions/ideas from anyone who has the time to look these changes over. Thanks so much!
          Hide
          Marshall McMullen added a comment -

          Finally sorted out how to properly run one unit test (e.g. MultiTransactionTest.java) from the command line and show its output:

          ant compile test -Dtest.category=MultiTransaction -Dtest.output=yes

          Show
          Marshall McMullen added a comment - Finally sorted out how to properly run one unit test (e.g. MultiTransactionTest.java) from the command line and show its output: ant compile test -Dtest.category=MultiTransaction -Dtest.output=yes
          Hide
          Marshall McMullen added a comment -

          Regarding my comments above, I've made great progress on #4. I am now able to collect out the result from each op in a multi op. If there's an exception on that op, it will get caught and stored into the results array. Ultimately I still throw an exception if any of the ops fail (just like before) but the change here is that we'll be able to determine which op failed and why.

          After I flush out more of these details I'll get this committed.

          Show
          Marshall McMullen added a comment - Regarding my comments above, I've made great progress on #4. I am now able to collect out the result from each op in a multi op. If there's an exception on that op, it will get caught and stored into the results array. Ultimately I still throw an exception if any of the ops fail (just like before) but the change here is that we'll be able to determine which op failed and why. After I flush out more of these details I'll get this committed.
          Hide
          Ted Dunning added a comment -

          This is awesome Marshall!

          Show
          Ted Dunning added a comment - This is awesome Marshall!
          Hide
          Marshall McMullen added a comment -

          Thanks Ted!! I'm super excited about this work as well. I'm working on flushing out some more details with failure detection and adding some unit tests around that. I'll commit them later today when they are finished....

          Next I want to tackle the client-side changes (Java and C). Any pointers on what file(s) to look into for where that's all handled would be appreciated...

          Thanks!!

          Show
          Marshall McMullen added a comment - Thanks Ted!! I'm super excited about this work as well. I'm working on flushing out some more details with failure detection and adding some unit tests around that. I'll commit them later today when they are finished.... Next I want to tackle the client-side changes (Java and C). Any pointers on what file(s) to look into for where that's all handled would be appreciated... Thanks!!
          Hide
          Ted Dunning added a comment -

          Marshall,

          Go ahead and micro-commit. Nobody is expecting the github branch to even pass tests.

          The client side (in Java) should be nearly done since that is where I started. We should be able to start writing
          some real unit tests that do round trip tests as soon as you are happy with the server stuff.

          Show
          Ted Dunning added a comment - Marshall, Go ahead and micro-commit. Nobody is expecting the github branch to even pass tests. The client side (in Java) should be nearly done since that is where I started. We should be able to start writing some real unit tests that do round trip tests as soon as you are happy with the server stuff.
          Hide
          Marshall McMullen added a comment -

          I've committed my work from the last couple of days that implements a mechanism to detect which op in the multi op failed. From my commit message:

          Enhanced multi-op so caller can tell which op failed.

          This turned out to be a lot harder than it looked. The jist of
          the problem was I was throwing an exception early in
          PrepRequestProcessor on the first error in the multi op. That was
          the right thing to do, but it didn't allow returning the results
          back to the caller as the exception caused us to discard the current
          multi-op and create a new ErrorTxn that traversed all the other
          request handlers.

          Instead, this new implementation retains the multi-op all the way
          through all the request handlers (though it becomes a MultiTxn
          and ultimately a MultiResponse). It now allows returning back
          to the caller the result for each individual Op.

          In ZooKeeper's multi function, it still ultimately behaves the
          same as before – namely the first error in the results array
          gets detected as the fatal error that caused the multi op to fail
          and this is what we throw as our exception.

          The major difference is that we first (optionally) populate a given
          result array before we throw the exception.

          There are some rough details here still and some things I'd like to
          fix. But it works really well and all the major bugs are flushed
          out so I'm committing....

          Show
          Marshall McMullen added a comment - I've committed my work from the last couple of days that implements a mechanism to detect which op in the multi op failed. From my commit message: Enhanced multi-op so caller can tell which op failed. This turned out to be a lot harder than it looked. The jist of the problem was I was throwing an exception early in PrepRequestProcessor on the first error in the multi op. That was the right thing to do, but it didn't allow returning the results back to the caller as the exception caused us to discard the current multi-op and create a new ErrorTxn that traversed all the other request handlers. Instead, this new implementation retains the multi-op all the way through all the request handlers (though it becomes a MultiTxn and ultimately a MultiResponse). It now allows returning back to the caller the result for each individual Op. In ZooKeeper's multi function, it still ultimately behaves the same as before – namely the first error in the results array gets detected as the fatal error that caused the multi op to fail and this is what we throw as our exception. The major difference is that we first (optionally) populate a given result array before we throw the exception. There are some rough details here still and some things I'd like to fix. But it works really well and all the major bugs are flushed out so I'm committing....
          Hide
          Marshall McMullen added a comment -

          Made a couple small bug fixes this morning:

          (1) Fix compile error introduced by new arg in multi_internal.
          (2) Fix bug in error reporting on failed multi-op.

          Now I'm looking at the C client. Hope to get this done today....

          Show
          Marshall McMullen added a comment - Made a couple small bug fixes this morning: (1) Fix compile error introduced by new arg in multi_internal. (2) Fix bug in error reporting on failed multi-op. Now I'm looking at the C client. Hope to get this done today....
          Hide
          Mahadev konar added a comment -

          nice work marshall. Given that 3.4 is getting delayed, if enough testing is done we can put it in 3.4.0 release as well!

          Show
          Mahadev konar added a comment - nice work marshall. Given that 3.4 is getting delayed, if enough testing is done we can put it in 3.4.0 release as well!
          Hide
          Marshall McMullen added a comment -

          Mahadev, that's wonderful news!! We would really like to get this into the 3.4.0 release as well. Do you know what the timeline is for the release of 3.4?

          Show
          Marshall McMullen added a comment - Mahadev, that's wonderful news!! We would really like to get this into the 3.4.0 release as well. Do you know what the timeline is for the release of 3.4?
          Hide
          Marshall McMullen added a comment -

          I've committed an initial pass at the necessary work in the C client. I did some refactoring to avoid some massive code duplication as well. Notes from my checkin comments:

          This adds new zoo_multi and zoo_amulti interfaces to the C client to
          atomically commit (either synchronously or asynchronously) a set of
          operations. The server side guarantees transactional consistency
          in that all the ops either fail or succeed.

          Limitations of this implementation:

          • asynchronous interface (zoo_amulti) doesn't work yet.
          • can't get back the index of the failing op from zoo_multi though
            zoo_amulti is designed to support this. I have a plan for how
            to implement this in zoo_multi too.
          • Only limited testing done on C client. The one test I did write
            only works in multithread mode. Looks to be a limitation of the
            test case I modelled mine after.

          Cleanup/refactoring needed:

          • Deal with deeply nested errors in zoo_amulti better (free any
            allocated memory, etc).
          • Don't need the existing ErrorResponse (added on java side to
            allow determining which op failed). On C side it was so much
            easier to just add a field to MultiHeader to indicate if that
            op failed or not. That will make the Java side a lot cleaner
          Show
          Marshall McMullen added a comment - I've committed an initial pass at the necessary work in the C client. I did some refactoring to avoid some massive code duplication as well. Notes from my checkin comments: This adds new zoo_multi and zoo_amulti interfaces to the C client to atomically commit (either synchronously or asynchronously) a set of operations. The server side guarantees transactional consistency in that all the ops either fail or succeed. Limitations of this implementation: asynchronous interface (zoo_amulti) doesn't work yet. can't get back the index of the failing op from zoo_multi though zoo_amulti is designed to support this. I have a plan for how to implement this in zoo_multi too. Only limited testing done on C client. The one test I did write only works in multithread mode. Looks to be a limitation of the test case I modelled mine after. Cleanup/refactoring needed: Deal with deeply nested errors in zoo_amulti better (free any allocated memory, etc). Don't need the existing ErrorResponse (added on java side to allow determining which op failed). On C side it was so much easier to just add a field to MultiHeader to indicate if that op failed or not. That will make the Java side a lot cleaner
          Hide
          Ted Dunning added a comment -

          Patch against current trunk. Commit messages are reworded versus github version

          Show
          Ted Dunning added a comment - Patch against current trunk. Commit messages are reworded versus github version
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12477971 ]
          Hide
          Ted Dunning added a comment -

          This patch should be very close to done for the Java side (thanks to Marshall and Camille for carrying this forward).

          Marshall is working out some kinks on the C API.

          Getting some reviews from the core ZK team would be very helpful at this point.

          Show
          Ted Dunning added a comment - This patch should be very close to done for the Java side (thanks to Marshall and Camille for carrying this forward). Marshall is working out some kinks on the C API. Getting some reviews from the core ZK team would be very helpful at this point.
          Hide
          Marshall McMullen added a comment -

          Ted, thanks for creating the patch against trunk.

          I should note that there are a few (previously mentioned) limitations/bugs with this version. I hope to sort them out over the next day or so. However, for anyone who wants to start reviewing this now is welcome to as the remaining bugs I intend to fix shouldn't modify the basic approach.

          Show
          Marshall McMullen added a comment - Ted, thanks for creating the patch against trunk. I should note that there are a few (previously mentioned) limitations/bugs with this version. I hope to sort them out over the next day or so. However, for anyone who wants to start reviewing this now is welcome to as the remaining bugs I intend to fix shouldn't modify the basic approach.
          Hide
          Marshall McMullen added a comment -

          Yes, I should clarify and confirm what Ted said. The Java side of this (both the client and server) should be very close to a final working version. The kinks I'm working out are definitely only on the C API side.

          Show
          Marshall McMullen added a comment - Yes, I should clarify and confirm what Ted said. The Java side of this (both the client and server) should be very close to a final working version. The kinks I'm working out are definitely only on the C API side.
          Hide
          Marshall McMullen added a comment -

          If anyone's looking at this already, please disregard the C client entirely. I've refactored it considerably after I got some sleep and took a second look at it. I realize now that I didn't understand the C client well enough when I wrote this first version. Now that I have more context, the solution was a lot simpler...

          Anyhow, I'll finish this up tonight and commit my changes back to Ted's git branch.

          Show
          Marshall McMullen added a comment - If anyone's looking at this already, please disregard the C client entirely. I've refactored it considerably after I got some sleep and took a second look at it. I realize now that I didn't understand the C client well enough when I wrote this first version. Now that I have more context, the solution was a lot simpler... Anyhow, I'll finish this up tonight and commit my changes back to Ted's git branch.
          Hide
          Mahadev konar added a comment -

          marshall,
          Great progress.
          you might want to post a preliminary patch for this on the jira for others to take a look at it.

          Show
          Mahadev konar added a comment - marshall, Great progress. you might want to post a preliminary patch for this on the jira for others to take a look at it.
          Hide
          Ted Dunning added a comment -

          Mahadev,

          There is a patch on this JIRA.

          Show
          Ted Dunning added a comment - Mahadev, There is a patch on this JIRA.
          Hide
          Ted Dunning added a comment -

          This patch isn't final yet.

          Show
          Ted Dunning added a comment - This patch isn't final yet.
          Ted Dunning made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Affects Version/s 3.3.3 [ 12315482 ]
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/236//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/12477971/ZOOKEEPER-965.patch against trunk revision 1097865. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/236//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Alright, made significant progress tonight rewriting my prior attempts at the C client. The new version is a lot cleaner, simpler, and safer. I still have a few things to work out, but it's working significantly better now than the prior version.

          We're moving offices tomorrow (I think, might be Wed..) but I'll try to get my latest changes committed as soon as I can.

          Show
          Marshall McMullen added a comment - Alright, made significant progress tonight rewriting my prior attempts at the C client. The new version is a lot cleaner, simpler, and safer. I still have a few things to work out, but it's working significantly better now than the prior version. We're moving offices tomorrow (I think, might be Wed..) but I'll try to get my latest changes committed as soon as I can.
          Hide
          Marshall McMullen added a comment -

          I've just committed a rewrite of the C API:
          https://github.com/tdunning/zookeeper/commit/dcd7ea9233314ce96c8913b8b0659a8f7d02e93b.

          From my commit message:

          commit dcd7ea9233314ce96c8913b8b0659a8f7d02e93b
          Author: Marshall McMullen <marshall@solidfire.com>
          Date: Tue May 3 17:45:00 2011 -0600

          ZOOKEEPER-965 - Rewrite of the C API for multi op support.

          This is a rewrite of my earlier C API support for multi op support.
          My earlier version didn't work correctly due to a lack of understanding
          of the completion mechanism and how it interacted with sync and async
          mechanisms. This version correctly works in both sync and async
          contexts.

          I've also gotten rid of the horrid variadic zoo_multi/zoo_amulti interfaces
          in favor of a simpler, stricter/safer, and actually more succinct and
          easier to use syntax. See test cases in TestClient.cc for examples.

          I've also added a couple of more tests cases to test the async
          interface and to test error code propagation.

          Removed no-longer-necessary cruft from my last version.

          Show
          Marshall McMullen added a comment - I've just committed a rewrite of the C API: https://github.com/tdunning/zookeeper/commit/dcd7ea9233314ce96c8913b8b0659a8f7d02e93b . From my commit message: commit dcd7ea9233314ce96c8913b8b0659a8f7d02e93b Author: Marshall McMullen <marshall@solidfire.com> Date: Tue May 3 17:45:00 2011 -0600 ZOOKEEPER-965 - Rewrite of the C API for multi op support. This is a rewrite of my earlier C API support for multi op support. My earlier version didn't work correctly due to a lack of understanding of the completion mechanism and how it interacted with sync and async mechanisms. This version correctly works in both sync and async contexts. I've also gotten rid of the horrid variadic zoo_multi/zoo_amulti interfaces in favor of a simpler, stricter/safer, and actually more succinct and easier to use syntax. See test cases in TestClient.cc for examples. I've also added a couple of more tests cases to test the async interface and to test error code propagation. Removed no-longer-necessary cruft from my last version.
          Hide
          Marshall McMullen added a comment -

          The rewrite of the C API's multi support exposed a bug in the backend multi op implementation that I must fix. I'll hopefully have a fix for that in hand late this evening.

          Show
          Marshall McMullen added a comment - The rewrite of the C API's multi support exposed a bug in the backend multi op implementation that I must fix. I'll hopefully have a fix for that in hand late this evening.
          Hide
          Ted Dunning added a comment -

          Updated to trunk and Marshall's changes

          Show
          Ted Dunning added a comment - Updated to trunk and Marshall's changes
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478111 ]
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/239//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/12478111/ZOOKEEPER-965.patch against trunk revision 1099267. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/239//console This message is automatically generated.
          Hide
          Ted Dunning added a comment -

          Hopefully this makes jenkins happier.

          Show
          Ted Dunning added a comment - Hopefully this makes jenkins happier.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478112 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 4 new Findbugs (version 1.3.9) warnings.

          -1 release audit. The applied patch generated 27 release audit warnings (more than the trunk's current 24 warnings).

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//testReport/
          Release audit warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//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/12478112/ZOOKEEPER-965.patch against trunk revision 1099267. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 4 new Findbugs (version 1.3.9) warnings. -1 release audit. The applied patch generated 27 release audit warnings (more than the trunk's current 24 warnings). -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//testReport/ Release audit warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/240//console This message is automatically generated.
          Hide
          Ted Dunning added a comment -

          Added some copyrights and cleaned up a bit of code.

          Show
          Ted Dunning added a comment - Added some copyrights and cleaned up a bit of code.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478117 ]
          Hide
          Marshall McMullen added a comment -

          I'm aware of the current failing tests. They're a result of the bug I referred to above. Specifically, on a failed multi op, a NULL MultiTransaction entry is getting written to disk. A subsequent attempt to read that back in fails. This bug wasn't found in the java unit tests, but it is found by my new C API unit tests. It's easy to reproduce and I have a good idea where the problem is. I'll get it fixed tonight.

          Show
          Marshall McMullen added a comment - I'm aware of the current failing tests. They're a result of the bug I referred to above. Specifically, on a failed multi op, a NULL MultiTransaction entry is getting written to disk. A subsequent attempt to read that back in fails. This bug wasn't found in the java unit tests, but it is found by my new C API unit tests. It's easy to reproduce and I have a good idea where the problem is. I'll get it fixed tonight.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/242//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/242//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/242//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/12478117/ZOOKEEPER-965.patch against trunk revision 1099267. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/242//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/242//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/242//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          So maybe someone can help me narrow in on where to find the problem I'm debugging. The C unit tests exposed a problem where it reads in the database snapshot from disk and finds a MultiTxn that is NULL. Here's the stacktrace:

          2011-05-03 21:07:11,609 [myid:] - INFO [main:FileSnap@83] - Reading snapshot /tmp/zkdata/version-2/snapshot.3e9
          2011-05-03 21:07:11,728 [myid:] - ERROR [main:ZooKeeperServerMain@63] - Unexpected exception, exiting abnormally
          java.lang.NullPointerException
          at org.apache.zookeeper.server.DataTree.processTxn(DataTree.java:818)
          at org.apache.zookeeper.server.persistence.FileTxnSnapLog.processTransaction(FileTxnSnapLog.java:186)
          at org.apache.zookeeper.server.persistence.FileTxnSnapLog.restore(FileTxnSnapLog.java:145)
          at org.apache.zookeeper.server.ZKDatabase.loadDataBase(ZKDatabase.java:223)
          at org.apache.zookeeper.server.ZooKeeperServer.loadData(ZooKeeperServer.java:242)
          at org.apache.zookeeper.server.ZooKeeperServer.startdata(ZooKeeperServer.java:369)
          at org.apache.zookeeper.server.NIOServerCnxnFactory.startup(NIOServerCnxnFactory.java:122)
          at org.apache.zookeeper.server.ZooKeeperServerMain.runFromConfig(ZooKeeperServerMain.java:112)
          at org.apache.zookeeper.server.ZooKeeperServerMain.initializeAndRun(ZooKeeperServerMain.java:86)
          at org.apache.zookeeper.server.ZooKeeperServerMain.main(ZooKeeperServerMain.java:52)

          A hack workaround is DataTree.processTxn, if the MultiTxn is NULL, just skip it. But that only addresses the symptom. I'd like to understand how it's getting into the snapshot that is being written to disk and NOT do so if it's an empty MultiTxn. I just don't know where all that magic happens... Anyone want to offer some guidance?

          Show
          Marshall McMullen added a comment - So maybe someone can help me narrow in on where to find the problem I'm debugging. The C unit tests exposed a problem where it reads in the database snapshot from disk and finds a MultiTxn that is NULL. Here's the stacktrace: 2011-05-03 21:07:11,609 [myid:] - INFO [main:FileSnap@83] - Reading snapshot /tmp/zkdata/version-2/snapshot.3e9 2011-05-03 21:07:11,728 [myid:] - ERROR [main:ZooKeeperServerMain@63] - Unexpected exception, exiting abnormally java.lang.NullPointerException at org.apache.zookeeper.server.DataTree.processTxn(DataTree.java:818) at org.apache.zookeeper.server.persistence.FileTxnSnapLog.processTransaction(FileTxnSnapLog.java:186) at org.apache.zookeeper.server.persistence.FileTxnSnapLog.restore(FileTxnSnapLog.java:145) at org.apache.zookeeper.server.ZKDatabase.loadDataBase(ZKDatabase.java:223) at org.apache.zookeeper.server.ZooKeeperServer.loadData(ZooKeeperServer.java:242) at org.apache.zookeeper.server.ZooKeeperServer.startdata(ZooKeeperServer.java:369) at org.apache.zookeeper.server.NIOServerCnxnFactory.startup(NIOServerCnxnFactory.java:122) at org.apache.zookeeper.server.ZooKeeperServerMain.runFromConfig(ZooKeeperServerMain.java:112) at org.apache.zookeeper.server.ZooKeeperServerMain.initializeAndRun(ZooKeeperServerMain.java:86) at org.apache.zookeeper.server.ZooKeeperServerMain.main(ZooKeeperServerMain.java:52) A hack workaround is DataTree.processTxn, if the MultiTxn is NULL, just skip it. But that only addresses the symptom. I'd like to understand how it's getting into the snapshot that is being written to disk and NOT do so if it's an empty MultiTxn. I just don't know where all that magic happens... Anyone want to offer some guidance?
          Hide
          Marshall McMullen added a comment -

          Additional debugging... Note that as zk server is coming up it's reading in a snapshot:

          2011-05-03 22:14:16,872 [myid:] - INFO [main:Environment@98] - Server environment:java.io.tmpdir=/tmp
          2011-05-03 22:14:16,872 [myid:] - INFO [main:Environment@98] - Server environment:java.compiler=<NA>
          2011-05-03 22:14:16,872 [myid:] - INFO [main:Environment@98] - Server environment:os.name=Linux
          2011-05-03 22:14:16,873 [myid:] - INFO [main:Environment@98] - Server environment:os.arch=amd64
          2011-05-03 22:14:16,873 [myid:] - INFO [main:Environment@98] - Server environment:os.version=2.6.37-gentoo-r4
          2011-05-03 22:14:16,874 [myid:] - INFO [main:Environment@98] - Server environment:user.name=marshall
          2011-05-03 22:14:16,874 [myid:] - INFO [main:Environment@98] - Server environment:user.home=/home/marshall
          2011-05-03 22:14:16,875 [myid:] - INFO [main:Environment@98] - Server environment:user.dir=/home/marshall/zookeeper-multi/src/c
          2011-05-03 22:14:16,880 [myid:] - INFO [main:ZooKeeperServer@713] - tickTime set to 3000
          2011-05-03 22:14:16,881 [myid:] - INFO [main:ZooKeeperServer@722] - minSessionTimeout set to -1
          2011-05-03 22:14:16,881 [myid:] - INFO [main:ZooKeeperServer@731] - maxSessionTimeout set to -1
          2011-05-03 22:14:16,899 [myid:] - INFO [main:NIOServerCnxnFactory@94] - binding to port 0.0.0.0/0.0.0.0:22181
          2011-05-03 22:14:16,916 [myid:] - INFO [main:FileSnap@83] - Reading snapshot /tmp/zkdata/version-2/snapshot.0
          2011-05-03 22:14:16,927 [myid:] - INFO [main:FileTxnSnapLog@186] - >> PROCESSING HDR=85490560384172033,1304482428,3,1304482430503,14 TXN=null

          This ends up triggering the null pointer exception b/c the TXN (which is of type MultiTxn) is NULL due to a failed multi-op. I'll try to track down better what the error handling is around a multi txn that fails and see why the Txn in the snapshot is null...

          Show
          Marshall McMullen added a comment - Additional debugging... Note that as zk server is coming up it's reading in a snapshot: 2011-05-03 22:14:16,872 [myid:] - INFO [main:Environment@98] - Server environment:java.io.tmpdir=/tmp 2011-05-03 22:14:16,872 [myid:] - INFO [main:Environment@98] - Server environment:java.compiler=<NA> 2011-05-03 22:14:16,872 [myid:] - INFO [main:Environment@98] - Server environment:os.name=Linux 2011-05-03 22:14:16,873 [myid:] - INFO [main:Environment@98] - Server environment:os.arch=amd64 2011-05-03 22:14:16,873 [myid:] - INFO [main:Environment@98] - Server environment:os.version=2.6.37-gentoo-r4 2011-05-03 22:14:16,874 [myid:] - INFO [main:Environment@98] - Server environment:user.name=marshall 2011-05-03 22:14:16,874 [myid:] - INFO [main:Environment@98] - Server environment:user.home=/home/marshall 2011-05-03 22:14:16,875 [myid:] - INFO [main:Environment@98] - Server environment:user.dir=/home/marshall/zookeeper-multi/src/c 2011-05-03 22:14:16,880 [myid:] - INFO [main:ZooKeeperServer@713] - tickTime set to 3000 2011-05-03 22:14:16,881 [myid:] - INFO [main:ZooKeeperServer@722] - minSessionTimeout set to -1 2011-05-03 22:14:16,881 [myid:] - INFO [main:ZooKeeperServer@731] - maxSessionTimeout set to -1 2011-05-03 22:14:16,899 [myid:] - INFO [main:NIOServerCnxnFactory@94] - binding to port 0.0.0.0/0.0.0.0:22181 2011-05-03 22:14:16,916 [myid:] - INFO [main:FileSnap@83] - Reading snapshot /tmp/zkdata/version-2/snapshot.0 2011-05-03 22:14:16,927 [myid:] - INFO [main:FileTxnSnapLog@186] - >> PROCESSING HDR=85490560384172033,1304482428,3,1304482430503,14 TXN=null This ends up triggering the null pointer exception b/c the TXN (which is of type MultiTxn) is NULL due to a failed multi-op. I'll try to track down better what the error handling is around a multi txn that fails and see why the Txn in the snapshot is null...
          Hide
          Mahadev konar added a comment -

          interesting, are you adding the multitxn in the c test? if no, this looks like a case wherein its reading an int written out via the txn log (not sure what) but when reading it back interprets it wrong. Which line is throwing null pointer exception (am assuming your codebase is different than the one on trunk).

          Show
          Mahadev konar added a comment - interesting, are you adding the multitxn in the c test? if no, this looks like a case wherein its reading an int written out via the txn log (not sure what) but when reading it back interprets it wrong. Which line is throwing null pointer exception (am assuming your codebase is different than the one on trunk).
          Hide
          Marshall McMullen added a comment -

          The C client is just calling the new zoo_multi API. One of the unit tests does a zoo_multi which purposefully fails (due to duplicate nodes). The error is correctly detected and propagated from server back to client. The NEXT unit tests which causes the server to start up again during it's init phase reads in the database off of the disk (ZKDatabase.loadDatabase) which then ultimately ends up in DataTree.processTxn(DataTree.java:818). That line of code is new code I've added to processTxn that essentially says:

          ...
          case OpCode.multi:
          MultiTxn multiTxn = (MultiTxn) txn ;
          List<Txn> txns = multiTxn.getTxns(); // <---- THAT CAUSES THE NULL POINTER EXCEPTION B/C 'txn' is null.

          So this bug only happens after a failed multi op. The normal multi op gets written to disk and loaded correctly. I don't understand all of how this works, but I presume we take a snapshot before we actually apply a transaction so we can roll back if it fails. So I think the snapshot we're taking is putting a null txn into the snapshot ... I'm not sure... Any ideas?

          Show
          Marshall McMullen added a comment - The C client is just calling the new zoo_multi API. One of the unit tests does a zoo_multi which purposefully fails (due to duplicate nodes). The error is correctly detected and propagated from server back to client. The NEXT unit tests which causes the server to start up again during it's init phase reads in the database off of the disk (ZKDatabase.loadDatabase) which then ultimately ends up in DataTree.processTxn(DataTree.java:818). That line of code is new code I've added to processTxn that essentially says: ... case OpCode.multi: MultiTxn multiTxn = (MultiTxn) txn ; List<Txn> txns = multiTxn.getTxns(); // <---- THAT CAUSES THE NULL POINTER EXCEPTION B/C 'txn' is null. So this bug only happens after a failed multi op. The normal multi op gets written to disk and loaded correctly. I don't understand all of how this works, but I presume we take a snapshot before we actually apply a transaction so we can roll back if it fails. So I think the snapshot we're taking is putting a null txn into the snapshot ... I'm not sure... Any ideas?
          Hide
          Marshall McMullen added a comment -

          *GAH* I'm stupid. OK, I found the problem... sigh. It had absolutely nothing to do with what I thought at all (typical).

          The snapshotting code serializes all the transactions to create a snapshot. While the code is written to serialize a MultiTxn properly, there was no case statement in the necessary place in SerializeUtils. So nothing was getting written out to the snapshot properly....

          sigh.

          Show
          Marshall McMullen added a comment - * GAH * I'm stupid. OK, I found the problem... sigh . It had absolutely nothing to do with what I thought at all (typical). The snapshotting code serializes all the transactions to create a snapshot. While the code is written to serialize a MultiTxn properly, there was no case statement in the necessary place in SerializeUtils. So nothing was getting written out to the snapshot properly.... sigh .
          Hide
          Marshall McMullen added a comment -

          Just checked in a fix. C unit tests now pass (including the three new multi-op tests). .

          Show
          Marshall McMullen added a comment - Just checked in a fix. C unit tests now pass (including the three new multi-op tests). .
          Hide
          Ted Dunning added a comment -

          Marshall,

          Do these C unit tests have a corresponding set of Java unit tests?

          Show
          Ted Dunning added a comment - Marshall, Do these C unit tests have a corresponding set of Java unit tests?
          Hide
          Marshall McMullen added a comment -

          The Java unit tests are actually much more developed (see MultiTransactionTest.java). Now that I've got all the plumbing in the C API working correctly, I'll work on porting the Java tests over to the C side....

          Show
          Marshall McMullen added a comment - The Java unit tests are actually much more developed (see MultiTransactionTest.java). Now that I've got all the plumbing in the C API working correctly, I'll work on porting the Java tests over to the C side....
          Hide
          Mahadev konar added a comment -

          nice catch! Good to see tests proving to be useful!

          Show
          Mahadev konar added a comment - nice catch! Good to see tests proving to be useful!
          Hide
          Ted Dunning added a comment -

          Latests repairs from Marshall.

          This should be ready for review, but not commit.

          Show
          Ted Dunning added a comment - Latests repairs from Marshall. This should be ready for review, but not commit.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478127 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//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/12478127/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          What is this warning about:

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.

          Show
          Marshall McMullen added a comment - What is this warning about: -1 findbugs. The patch appears to introduce 2 new Findbugs (version 1.3.9) warnings.
          Hide
          Camille Fournier added a comment -

          Check out the findbugs link:
          https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html

          Looks like they're upset by:
          org.apache.zookeeper.Op$Create.getData() may expose internal representation by returning Op$Create.data
          In class org.apache.zookeeper.Op$Create
          In method org.apache.zookeeper.Op$Create.getData()
          Field org.apache.zookeeper.Op$Create.data
          At Op.java:[line 99]

          org.apache.zookeeper.Op$SetData.getData() may expose internal representation by returning Op$SetData.data

          Show
          Camille Fournier added a comment - Check out the findbugs link: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/245//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Looks like they're upset by: org.apache.zookeeper.Op$Create.getData() may expose internal representation by returning Op$Create.data In class org.apache.zookeeper.Op$Create In method org.apache.zookeeper.Op$Create.getData() Field org.apache.zookeeper.Op$Create.data At Op.java: [line 99] org.apache.zookeeper.Op$SetData.getData() may expose internal representation by returning Op$SetData.data
          Hide
          Marshall McMullen added a comment -

          Thanks Camille! I've got some other things on my plate today, but I'll try to get to this later.

          Show
          Marshall McMullen added a comment - Thanks Camille! I've got some other things on my plate today, but I'll try to get to this later.
          Hide
          Ted Dunning added a comment -

          Camille is correct.

          I don't think that we can get rid of these warnings because passing the buffer around here is of the essence. The warning
          is reasonable except for the fact that the data structure is really just a wrapper around the byte array.

          Mahadev, Ben,

          Is it OK to ultimately commit code that has these two warnings?

          Show
          Ted Dunning added a comment - Camille is correct. I don't think that we can get rid of these warnings because passing the buffer around here is of the essence. The warning is reasonable except for the fact that the data structure is really just a wrapper around the byte array. Mahadev, Ben, Is it OK to ultimately commit code that has these two warnings?
          Hide
          Marshall McMullen added a comment -

          Actually I have a solution to this (I believe). Basically put a constructor inside Op.java that takes a Record and creates an Op. Then we don't have to expose the internals of the data field to the caller. I should have something in the next hour or so.

          Show
          Marshall McMullen added a comment - Actually I have a solution to this (I believe). Basically put a constructor inside Op.java that takes a Record and creates an Op. Then we don't have to expose the internals of the data field to the caller. I should have something in the next hour or so.
          Hide
          Marshall McMullen added a comment -

          Or, the 'add' method just takes a type and a request and does the right thing there. No internals exposed.

          Show
          Marshall McMullen added a comment - Or, the 'add' method just takes a type and a request and does the right thing there. No internals exposed.
          Hide
          Marshall McMullen added a comment -

          OK, Looks like this approach will work to get rid of this error.

          Show
          Marshall McMullen added a comment - OK, Looks like this approach will work to get rid of this error.
          Hide
          Marshall McMullen added a comment -

          Letting this test run completely but I'll check in a fix momentarily.

          Show
          Marshall McMullen added a comment - Letting this test run completely but I'll check in a fix momentarily.
          Hide
          Ted Dunning added a comment -

          There is a much simpler approach.

          ALL of the getters can be deleted and the toRequestRecord method can be used. THis also kills several casts.

          Patch coming shortly.

          Show
          Ted Dunning added a comment - There is a much simpler approach. ALL of the getters can be deleted and the toRequestRecord method can be used. THis also kills several casts. Patch coming shortly.
          Hide
          Marshall McMullen added a comment -

          Yeah, I was looking at using that as well. I'll let you tackle this then. Let me know if you need help...

          Show
          Marshall McMullen added a comment - Yeah, I was looking at using that as well. I'll let you tackle this then. Let me know if you need help...
          Hide
          Ted Dunning added a comment -

          Pushed fix to github.

          Note that I also reworded the commit messages to include the JIRA.

          This will require that you force the pull.

          Show
          Ted Dunning added a comment - Pushed fix to github. Note that I also reworded the commit messages to include the JIRA. This will require that you force the pull.
          Hide
          Ted Dunning added a comment -

          I made the tweaks. I will post another patch and see what jenkins has to say.

          Show
          Ted Dunning added a comment - I made the tweaks. I will post another patch and see what jenkins has to say.
          Hide
          Ted Dunning added a comment -

          This should fix the niggling warnings

          Show
          Ted Dunning added a comment - This should fix the niggling warnings
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478186 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/248//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/248//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/248//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/12478186/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/248//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/248//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/248//console This message is automatically generated.
          Hide
          Ted Dunning added a comment -

          This latest Hudson report appears to be a Jenkins problem rather than a real test failure.

          Show
          Ted Dunning added a comment - This latest Hudson report appears to be a Jenkins problem rather than a real test failure.
          Hide
          Marshall McMullen added a comment -

          I've just checked in a change to allow setdata ops to work properly. Previously I was using a struct with a union of all the op types. This worked well on C as you can specify which field of union to initialize to using extended struct initializers. This isn't supported on C++ without special compiler flags so I decided that was a bad approach. So I opted for a much simpler solution. Put all the necessary arguments into the main op_t structure and provide some simple macros that provide simple convenience wrappers to only take the arguments appropriate for that op (and in the same order as the non-multi version). This provides very simple caller syntax with no added overhead or memory allocations.

          I also added an entirely new C API test suite that's a port of the tests from the Java Client side.

          There is still a limitation on C API setdata op in that it can't pass back Stat structure results to caller. This is causing two of my new tests to fail so that part of the tests are commented out for now.

          Show
          Marshall McMullen added a comment - I've just checked in a change to allow setdata ops to work properly. Previously I was using a struct with a union of all the op types. This worked well on C as you can specify which field of union to initialize to using extended struct initializers. This isn't supported on C++ without special compiler flags so I decided that was a bad approach. So I opted for a much simpler solution. Put all the necessary arguments into the main op_t structure and provide some simple macros that provide simple convenience wrappers to only take the arguments appropriate for that op (and in the same order as the non-multi version). This provides very simple caller syntax with no added overhead or memory allocations. I also added an entirely new C API test suite that's a port of the tests from the Java Client side. There is still a limitation on C API setdata op in that it can't pass back Stat structure results to caller. This is causing two of my new tests to fail so that part of the tests are commented out for now.
          Hide
          Ted Dunning added a comment -

          This is a WIP patch. Marshall says that he has a setData issue still that is giving a bit of pause.

          This is still a very good reference for review.

          Show
          Ted Dunning added a comment - This is a WIP patch. Marshall says that he has a setData issue still that is giving a bit of pause. This is still a very good reference for review.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478618 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/252//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/252//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/252//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/12478618/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/252//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/252//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/252//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          I've just committed a change to fix the bugs with the C API and a setdata op inside a multi op. It was mostly due to bugs in the test code, but it did identify a real bug inside zookeeper.c so I'm glad I finally tracked it down.

          Everything should be pretty stable and workable now. Comments regarding the current state:

          (1) There are a few "FIXME" comments in the code I've checked in that I'd like input from anyone who reviews
          (2) C API unit tests don't work in single-threaded mode. I believe this is due to limitations in the unit test I copied as starting point for my unit test. Would love any input on this if anyone can take a look at what I'm doing wrong there.
          (3) The Java API supports a "CHECK_OP" inside the multi-op but the C API does not. I will try to implement that soon.

          Show
          Marshall McMullen added a comment - I've just committed a change to fix the bugs with the C API and a setdata op inside a multi op. It was mostly due to bugs in the test code, but it did identify a real bug inside zookeeper.c so I'm glad I finally tracked it down. Everything should be pretty stable and workable now. Comments regarding the current state: (1) There are a few "FIXME" comments in the code I've checked in that I'd like input from anyone who reviews (2) C API unit tests don't work in single-threaded mode. I believe this is due to limitations in the unit test I copied as starting point for my unit test. Would love any input on this if anyone can take a look at what I'm doing wrong there. (3) The Java API supports a "CHECK_OP" inside the multi-op but the C API does not. I will try to implement that soon.
          Hide
          Ted Dunning added a comment -

          Here is Marshall's code in the form of an updated patch against trunk

          Show
          Ted Dunning added a comment - Here is Marshall's code in the form of an updated patch against trunk
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478667 ]
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/253//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/12478667/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/253//console This message is automatically generated.
          Hide
          Ted Dunning added a comment -

          Second try getting a usable diff.

          Show
          Ted Dunning added a comment - Second try getting a usable diff.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478668 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/254//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/254//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/254//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/12478668/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/254//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/254//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/254//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          In regards to my comment above: "(3) The Java API supports a "CHECK_OP" inside the multi-op but the C API does not. I will try to implement that soon." I've just checked in a fix for this. And actually, the Java server and client side for CHECK_OP (CheckVersionRequest) weren't fully implemented either. I flushed out all the details and got it working correctly on both sides.

          The C API allows passing in an optional pointer for the Stat structure to get back the stat of the node in question. Or just pass in NULL if you only care about doing the version check itself.

          Show
          Marshall McMullen added a comment - In regards to my comment above: "(3) The Java API supports a "CHECK_OP" inside the multi-op but the C API does not. I will try to implement that soon." I've just checked in a fix for this. And actually, the Java server and client side for CHECK_OP (CheckVersionRequest) weren't fully implemented either. I flushed out all the details and got it working correctly on both sides. The C API allows passing in an optional pointer for the Stat structure to get back the stat of the node in question. Or just pass in NULL if you only care about doing the version check itself.
          Hide
          Marshall McMullen added a comment -

          Pending comments and feedback, I think after Ted builds a new patch against my latest commit (https://github.com/tdunning/zookeeper/commit/9da6439682c1bc23ca922473951f37e02c5a73b7) then I think I'm done with this feature... Would love folks to start looking it over and give me some feedback.

          Thanks!

          Show
          Marshall McMullen added a comment - Pending comments and feedback, I think after Ted builds a new patch against my latest commit ( https://github.com/tdunning/zookeeper/commit/9da6439682c1bc23ca922473951f37e02c5a73b7 ) then I think I'm done with this feature... Would love folks to start looking it over and give me some feedback. Thanks!
          Hide
          Marshall McMullen added a comment -

          Just added a new commit to Ted' branch to fix check op's in a non-multi context. From my commit message:

          ZOOKEEPER-965 - Fix CHECK_OP to work in a non-multi context.

          The CHECK_OP wasn't implemented to work in a non-multi context. It was added
          specifically for multi-op, but for completeness and consistency I felt it was
          better to go ahead and support it in a non-multi context.

          Also added some unit test code around the multi-op version of a CHECK_OP as
          well as a non-multi op versio nof CHECK_OP.

          Also fixed/added comments to zookeper.h regarding new APIs.

          Show
          Marshall McMullen added a comment - Just added a new commit to Ted' branch to fix check op's in a non-multi context. From my commit message: ZOOKEEPER-965 - Fix CHECK_OP to work in a non-multi context. The CHECK_OP wasn't implemented to work in a non-multi context. It was added specifically for multi-op, but for completeness and consistency I felt it was better to go ahead and support it in a non-multi context. Also added some unit test code around the multi-op version of a CHECK_OP as well as a non-multi op versio nof CHECK_OP. Also fixed/added comments to zookeper.h regarding new APIs.
          Hide
          Ted Dunning added a comment -

          Includes Marshall's latest updates.

          Show
          Ted Dunning added a comment - Includes Marshall's latest updates.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12478739 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/257//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/257//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/257//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/12478739/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/257//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/257//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/257//console This message is automatically generated.
          Hide
          Stephen Tyree added a comment -

          Great job all around! Some comments on the C library portion of the patch:

          • The zoo_acheck comment includes documentation on a member 'stat' that is not an argument
          • The zoo_check comment has the parameter stat listed as _stat
          • In create_completion_entry, you commented out the else case where you set c->clist to NULL. Did you mean to leave this in there?
          • In op_result_string_completion, you memcpy from the value onto data->value based on the size of value. Is data->value guaranteed to be larger than value?
          • You only assert result in op_result_string_completion, but none of the other op_result_* functions
          • Do you not need to set result->err in op_result_multi_completion?
          • In zoo_amulti, any reason "int index = 0;" isn't a few lines down right before it's used?
          • Slick zoo_amulti code, very nicely done.
          • In testCreate you compare results[*].err to 0, which will work, but shouldn't you compare it to ZOK? Or do I misunderstand op_result_t.
          • In testDelete, your comment should say "// '/multi2' should have been deleted"

          All-in-all the code looks great in the C client. Nicely done.

          Show
          Stephen Tyree added a comment - Great job all around! Some comments on the C library portion of the patch: The zoo_acheck comment includes documentation on a member 'stat' that is not an argument The zoo_check comment has the parameter stat listed as _stat In create_completion_entry, you commented out the else case where you set c->clist to NULL. Did you mean to leave this in there? In op_result_string_completion, you memcpy from the value onto data->value based on the size of value. Is data->value guaranteed to be larger than value? You only assert result in op_result_string_completion, but none of the other op_result_* functions Do you not need to set result->err in op_result_multi_completion? In zoo_amulti, any reason "int index = 0;" isn't a few lines down right before it's used? Slick zoo_amulti code, very nicely done. In testCreate you compare results [*] .err to 0, which will work, but shouldn't you compare it to ZOK? Or do I misunderstand op_result_t. In testDelete, your comment should say "// '/multi2' should have been deleted" All-in-all the code looks great in the C client. Nicely done.
          Hide
          Ted Dunning added a comment -

          Stephen,

          Nice work on the code review. Marshall will have to comment on your points, but it is clear that you
          read the code well and deserve a stroke for that!

          Show
          Ted Dunning added a comment - Stephen, Nice work on the code review. Marshall will have to comment on your points, but it is clear that you read the code well and deserve a stroke for that!
          Hide
          Marshall McMullen added a comment -

          Stephen,

          Thanks very much for doing a thorough code review of the C library. It is super appreciated. I think all your points are valid. Specifically, comments in-line below:

          • The zoo_acheck comment includes documentation on a member 'stat' that is not an argument
          • The zoo_check comment has the parameter stat listed as _stat

          Copy-paste errors . Fixed.

          • In create_completion_entry, you commented out the else case where you set c->clist to NULL. Did you mean to leave this in there?

          I had at one point had that field as a pointer instead of a struct. Instead I should probably zero out that structure to be safest. It shouldn't matter in this case, but it's better to be overly cautious I think.

          • In op_result_string_completion, you memcpy from the value onto data->value based on the size of value. Is data->value guaranteed to be larger than value?

          No, that's a very good catch. I need to take the length into account and deal with that error path accordingly as we do elsewhere. I'll check in a fix for this.

          • You only assert result in op_result_string_completion, but none of the other op_result_* functions

          Nice catch. I'll add that to my fixes.

          • Do you not need to set result->err in op_result_multi_completion?

          No, I don't think so. Unlike the other op_result_* completions, this one doesn't take a op_result_t that I fill in the return code. This purpose of this completion is to hold the list of completions for each op in the multi op. That way when we see a multi op, we can pop off the completions and call them. You may have noticed I append a COMPLETION_VOID (I called this the "tail completion" at the end of the multi's completion list. This is a void completion that does set the return value in the completion so it propagates to the caller. Does that make sense? I'll add some comments to the completion to explain.

          • In zoo_amulti, any reason "int index = 0;" isn't a few lines down right before it's used?

          Nope, probably result of moving stuff around a lot. I'll clean that up.

          • Slick zoo_amulti code, very nicely done.

          :-] THANKS!! I wrestled with the implementation for a long time. I'm happy with how it came out.

          • In testCreate you compare results[*].err to 0, which will work, but shouldn't you compare it to ZOK? Or do I misunderstand op_result_t.

          Yes, you're right. I also should reverse those arguments to CPPUNIT_ASSERT_EQUAL as we always have the expected followed by the actual. I also found that error elsewhere in that test file that I fixed.

          • In testDelete, your comment should say "// '/multi2' should have been deleted"

          Yep, good catch.

          Again, thanks for all the valuable and appreciate feedback. I'll commit these changes shortly.

          Show
          Marshall McMullen added a comment - Stephen, Thanks very much for doing a thorough code review of the C library. It is super appreciated. I think all your points are valid. Specifically, comments in-line below: The zoo_acheck comment includes documentation on a member 'stat' that is not an argument The zoo_check comment has the parameter stat listed as _stat Copy-paste errors . Fixed. In create_completion_entry, you commented out the else case where you set c->clist to NULL. Did you mean to leave this in there? I had at one point had that field as a pointer instead of a struct. Instead I should probably zero out that structure to be safest. It shouldn't matter in this case, but it's better to be overly cautious I think. In op_result_string_completion, you memcpy from the value onto data->value based on the size of value. Is data->value guaranteed to be larger than value? No, that's a very good catch. I need to take the length into account and deal with that error path accordingly as we do elsewhere. I'll check in a fix for this. You only assert result in op_result_string_completion, but none of the other op_result_* functions Nice catch. I'll add that to my fixes. Do you not need to set result->err in op_result_multi_completion? No, I don't think so. Unlike the other op_result_* completions, this one doesn't take a op_result_t that I fill in the return code. This purpose of this completion is to hold the list of completions for each op in the multi op. That way when we see a multi op, we can pop off the completions and call them. You may have noticed I append a COMPLETION_VOID (I called this the "tail completion" at the end of the multi's completion list. This is a void completion that does set the return value in the completion so it propagates to the caller. Does that make sense? I'll add some comments to the completion to explain. In zoo_amulti, any reason "int index = 0;" isn't a few lines down right before it's used? Nope, probably result of moving stuff around a lot. I'll clean that up. Slick zoo_amulti code, very nicely done. :-] THANKS!! I wrestled with the implementation for a long time. I'm happy with how it came out. In testCreate you compare results [*] .err to 0, which will work, but shouldn't you compare it to ZOK? Or do I misunderstand op_result_t. Yes, you're right. I also should reverse those arguments to CPPUNIT_ASSERT_EQUAL as we always have the expected followed by the actual. I also found that error elsewhere in that test file that I fixed. In testDelete, your comment should say "// '/multi2' should have been deleted" Yep, good catch. Again, thanks for all the valuable and appreciate feedback. I'll commit these changes shortly.
          Hide
          Marshall McMullen added a comment -

          Ted, I pushed my latest changes to fix the things Stephen identified in his review.

          Show
          Marshall McMullen added a comment - Ted, I pushed my latest changes to fix the things Stephen identified in his review.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/
          -----------------------------------------------------------

          Review request for zookeeper and Benjamin Reed.

          Summary
          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to
          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.
          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs


          CHANGES.txt d22f1f0
          src/c/Makefile.am 65830fe
          src/c/configure.ac cdea898
          src/c/include/proto.h 843032f
          src/c/include/zookeeper.h c055edb
          src/c/src/zookeeper.c db715b0
          src/c/tests/TestMulti.cc PRE-CREATION
          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION
          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION
          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION
          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f
          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012
          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7
          src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java 68970d2
          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d
          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2
          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff
          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929
          src/java/main/org/apache/zookeeper/server/package.html 3ec7656
          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5
          src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 9774fb2
          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6
          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION
          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing
          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- Review request for zookeeper and Benjamin Reed. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs CHANGES.txt d22f1f0 src/c/Makefile.am 65830fe src/c/configure.ac cdea898 src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java 68970d2 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 9774fb2 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Ted Dunning added a comment -

          Latest updates from Marshall. This is also the same as the patch for the review request that I just posted on RB.

          Show
          Ted Dunning added a comment - Latest updates from Marshall. This is also the same as the patch for the review request that I just posted on RB.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12479027 ]
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/259//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/12479027/ZOOKEEPER-965.patch against trunk revision 1099329. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/259//console This message is automatically generated.
          Hide
          Marshall McMullen added a comment -

          Ted, are you going to look at why this patch wouldn't apply cleanly or do you want me to investigate? Thanks!

          Show
          Marshall McMullen added a comment - Ted, are you going to look at why this patch wouldn't apply cleanly or do you want me to investigate? Thanks!
          Hide
          Ted Dunning added a comment -

          I will take a look. It is likely the prefix issue.

          Show
          Ted Dunning added a comment - I will take a look. It is likely the prefix issue.
          Hide
          Marshall McMullen added a comment -

          Ted, I pushed another update to your branch. The macro names in proto.h didn't adhere to normal zookeeper naming convention of prefixing everything (externally visible) with ZOO_. That caused conflicts when including other packages that already used "CHECK_OP" (which I ran into today). So I renamed them all as well as the places they were used to use the right naming convention.

          Note: This won't break any compatibility anywhere as we were never exposing this list of defines before the multi op code was added.

          Show
          Marshall McMullen added a comment - Ted, I pushed another update to your branch. The macro names in proto.h didn't adhere to normal zookeeper naming convention of prefixing everything (externally visible) with ZOO_. That caused conflicts when including other packages that already used "CHECK_OP" (which I ran into today). So I renamed them all as well as the places they were used to use the right naming convention. Note: This won't break any compatibility anywhere as we were never exposing this list of defines before the multi op code was added.
          Hide
          Ted Dunning added a comment -

          Great.

          I will use this as an excuse to jump on the problem with the patch again.

          On Sat, May 14, 2011 at 8:09 PM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - Great. I will use this as an excuse to jump on the problem with the patch again. On Sat, May 14, 2011 at 8:09 PM, Marshall McMullen (JIRA)
          Hide
          Ted Dunning added a comment -

          Name changes in the C code only.

          Show
          Ted Dunning added a comment - Name changes in the C code only.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12479254 ]
          Hide
          Marshall McMullen added a comment -

          In line with my prior change to rename externally visible macros to have ZOO_ prefix, I've done the same to op_t and op_result_t so they are now zoo_op_t and zoo_op_result_t.

          Name changes in the C code only but important to avoid namespace pollution.

          Show
          Marshall McMullen added a comment - In line with my prior change to rename externally visible macros to have ZOO_ prefix, I've done the same to op_t and op_result_t so they are now zoo_op_t and zoo_op_result_t. Name changes in the C code only but important to avoid namespace pollution.
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/265//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/12479254/ZOOKEEPER-965.patch against trunk revision 1103811. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/265//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          i'm reviewing the patch. you guys did a lot of great work. i have a couple of questions. i see what is happening in the code, but i want to make sure i understand the semantics:

          1) if i pass a list of multi ops and there is an error, does it all fail?
          2) what is the use case for the check operation?

          Show
          Benjamin Reed added a comment - i'm reviewing the patch. you guys did a lot of great work. i have a couple of questions. i see what is happening in the code, but i want to make sure i understand the semantics: 1) if i pass a list of multi ops and there is an error, does it all fail? 2) what is the use case for the check operation?
          Hide
          Ted Dunning added a comment -

          Yes.

          Imagine a graph where each node is a znode. If you want to update a node
          based on the contents of its neighbors, you would first read the list of
          neighbors from the node, then read each of the neighbors contents, and then
          finally update the original node. You might not want to complete the
          update, however, if one of the neighbors had been modified in the meantime.

          To accomplish this, you would include check operations for each of neighbors
          in your update as well as a version for the node being updated.

          This is essentially a multi-node extension of the standard
          read-modify-write-if-correct-version idiom widely used in normal ZK work.

          Show
          Ted Dunning added a comment - Yes. Imagine a graph where each node is a znode. If you want to update a node based on the contents of its neighbors, you would first read the list of neighbors from the node, then read each of the neighbors contents, and then finally update the original node. You might not want to complete the update, however, if one of the neighbors had been modified in the meantime. To accomplish this, you would include check operations for each of neighbors in your update as well as a version for the node being updated. This is essentially a multi-node extension of the standard read-modify-write-if-correct-version idiom widely used in normal ZK work.
          Hide
          Marshall McMullen added a comment -

          Benjamin,

          Thanks very much!! This was a lot of work, but also very interesting. Really glad to have been able to jump in and work on it. As to your questions:

          1) Yes, if there is an error in any of the ops in the multi-op, then the entire multi-op fails. On the java client, this will throw an exception corresponding to the error in the op that caused the multi-op to fail. You also optionally get an array populated with the results of each op in the multi-op in case you want to iterate over it and see which one failed. The semantics of the error codes in that result array are as follows

          ZOK: the op would have succeeded
          ZRUNTIMEINCONSISTENCY: We never tried the op because it was after the op that caused the multi-op to fail. I chose this error code (somewhat arbitrarily) because it would violate transactional consistency if we were to have committed an op inside a failing multi-op.
          Anything else: The error of the failing op.

          2) Ted, feel free to jump in here, but I believe the idea is to not proceed with the ops that follow in a multi op if a version on some other node has changed out from under us. e.g. if you have 100 ops in a multi op, and you get to the 50th one, it's a way to do a sanity check of "hey, don't go past this point if any other client changed this node"... Does that make sense?

          Show
          Marshall McMullen added a comment - Benjamin, Thanks very much!! This was a lot of work, but also very interesting. Really glad to have been able to jump in and work on it. As to your questions: 1) Yes, if there is an error in any of the ops in the multi-op, then the entire multi-op fails. On the java client, this will throw an exception corresponding to the error in the op that caused the multi-op to fail. You also optionally get an array populated with the results of each op in the multi-op in case you want to iterate over it and see which one failed. The semantics of the error codes in that result array are as follows ZOK: the op would have succeeded ZRUNTIMEINCONSISTENCY: We never tried the op because it was after the op that caused the multi-op to fail. I chose this error code (somewhat arbitrarily) because it would violate transactional consistency if we were to have committed an op inside a failing multi-op. Anything else: The error of the failing op. 2) Ted, feel free to jump in here, but I believe the idea is to not proceed with the ops that follow in a multi op if a version on some other node has changed out from under us. e.g. if you have 100 ops in a multi op, and you get to the 50th one, it's a way to do a sanity check of "hey, don't go past this point if any other client changed this node"... Does that make sense?
          Hide
          Marshall McMullen added a comment -

          Also, regarding #1 above... on the C client, we don't throw exceptions obviously. The C client returns the error of the failing op in the multi-op. The semantics are the same though as it takes an optional array to fill in with the results of all the ops in the multiop. This array can be used like in the Java client to see which op caused the multi op to fail...

          Show
          Marshall McMullen added a comment - Also, regarding #1 above... on the C client, we don't throw exceptions obviously. The C client returns the error of the failing op in the multi-op. The semantics are the same though as it takes an optional array to fill in with the results of all the ops in the multiop. This array can be used like in the Java client to see which op caused the multi op to fail...
          Hide
          Benjamin Reed added a comment -

          nice design. ok back to reviewing

          Show
          Benjamin Reed added a comment - nice design. ok back to reviewing
          Hide
          Camille Fournier added a comment -

          It would be really great if we could get https://issues.apache.org/jira/browse/ZOOKEEPER-1046 merged in before this patch. This feature will require a few additional changes on 1046 but given that 1046 is an outstanding bug I'd like to see it fixed before we add the feature.

          Show
          Camille Fournier added a comment - It would be really great if we could get https://issues.apache.org/jira/browse/ZOOKEEPER-1046 merged in before this patch. This feature will require a few additional changes on 1046 but given that 1046 is an outstanding bug I'd like to see it fixed before we add the feature.
          Hide
          Marshall McMullen added a comment -

          Ted, others...

          I've just pushed a commit to Ted's private branch to fix a few minor error handling issues I discovered while integrating this into our own product. Details below from my commit message:

          ZOOKEEPER-965 - Fix error code handling.

          Error codes were not propagating out from the server properly.
          The original design dictated that if the multi op fails, then
          all the entries in the multi result would be an error result. The
          error code in that error result would be used to determine which
          op failed in the multi-op. Specifically, error codes in the multi result:

          0: The op would have succeeded had the entire multi op not failed
          ZRUNTIMEINCONSISTENCY: The op was AFTER the failing op and was aborted
          Anything else: The real error code for the op that caused the entire
          multi op to fail (this is the one that is thrown as well).

          Also, discovered another error code issue. If a op in the multi
          op is after the failing op, it still ran through PrepRequestProcessor.
          That was confusing (and unecessary) as we know the entire multi op
          is going to fail. Modified it to abort all ops after a failing op
          to avoid unecessary work and also to avoid confusion in the log files.

          Also added some unit tests to verify all above described changes.

          Show
          Marshall McMullen added a comment - Ted, others... I've just pushed a commit to Ted's private branch to fix a few minor error handling issues I discovered while integrating this into our own product. Details below from my commit message: ZOOKEEPER-965 - Fix error code handling. Error codes were not propagating out from the server properly. The original design dictated that if the multi op fails, then all the entries in the multi result would be an error result. The error code in that error result would be used to determine which op failed in the multi-op. Specifically, error codes in the multi result: 0: The op would have succeeded had the entire multi op not failed ZRUNTIMEINCONSISTENCY: The op was AFTER the failing op and was aborted Anything else: The real error code for the op that caused the entire multi op to fail (this is the one that is thrown as well). Also, discovered another error code issue. If a op in the multi op is after the failing op, it still ran through PrepRequestProcessor. That was confusing (and unecessary) as we know the entire multi op is going to fail. Modified it to abort all ops after a failing op to avoid unecessary work and also to avoid confusion in the log files. Also added some unit tests to verify all above described changes.
          Hide
          Ted Dunning added a comment -

          Patch will be updated shortly. I will also update reviewboard if I hear
          from somebody that this will not damage ongoing reviews.

          Also, the "private branch" mentioned below is available to anyone at
          https://github.com/tdunning/zookeeper

          On Wed, May 18, 2011 at 12:56 PM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - Patch will be updated shortly. I will also update reviewboard if I hear from somebody that this will not damage ongoing reviews. Also, the "private branch" mentioned below is available to anyone at https://github.com/tdunning/zookeeper On Wed, May 18, 2011 at 12:56 PM, Marshall McMullen (JIRA)
          Hide
          Benjamin Reed added a comment -

          please update. i'm still digging into how everything fits together, so i haven't really started any significant comments.

          i do have a general comment. we need to get a bit more documentation in both javadoc and forrest. (i'm i remembering correctly that there was some doxygen?)

          Show
          Benjamin Reed added a comment - please update. i'm still digging into how everything fits together, so i haven't really started any significant comments. i do have a general comment. we need to get a bit more documentation in both javadoc and forrest. (i'm i remembering correctly that there was some doxygen?)
          Hide
          Ted Dunning added a comment -

          Updated to trunk. Includes Marshall's latest naming tweaks.

          Show
          Ted Dunning added a comment - Updated to trunk. Includes Marshall's latest naming tweaks.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12479708 ]
          Hide
          Ted Dunning added a comment -

          Updated review board

          Show
          Ted Dunning added a comment - Updated review board
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/
          -----------------------------------------------------------

          (Updated 2011-05-19 01:35:29.152433)

          Review request for zookeeper and Benjamin Reed.

          Changes
          -------

          Latest small changes from Marshall.
          Updated to trunk.

          Summary
          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to
          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.
          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs (updated)


          src/c/Makefile.am 65830fe
          src/c/include/proto.h 843032f
          src/c/include/zookeeper.h c055edb
          src/c/src/zookeeper.c db715b0
          src/c/tests/TestMulti.cc PRE-CREATION
          src/c/tests/TestOperations.cc f9441ea
          src/c/tests/ZKMocks.cc a75dce6
          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION
          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION
          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION
          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f
          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012
          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7
          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d
          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2
          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff
          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929
          src/java/main/org/apache/zookeeper/server/package.html 3ec7656
          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5
          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6
          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION
          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing
          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-19 01:35:29.152433) Review request for zookeeper and Benjamin Reed. Changes ------- Latest small changes from Marshall. Updated to trunk. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs (updated) src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Ted Dunning added a comment -

          Added some javadocs per request.

          Show
          Ted Dunning added a comment - Added some javadocs per request.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12479713 ]
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/
          -----------------------------------------------------------

          (Updated 2011-05-19 02:13:14.619335)

          Review request for zookeeper and Benjamin Reed.

          Changes
          -------

          Added javadocs

          Summary
          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to
          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.
          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs (updated)


          src/c/Makefile.am 65830fe
          src/c/include/proto.h 843032f
          src/c/include/zookeeper.h c055edb
          src/c/src/zookeeper.c db715b0
          src/c/tests/TestMulti.cc PRE-CREATION
          src/c/tests/TestOperations.cc f9441ea
          src/c/tests/ZKMocks.cc a75dce6
          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION
          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION
          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION
          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f
          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012
          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7
          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d
          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2
          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff
          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929
          src/java/main/org/apache/zookeeper/server/package.html 3ec7656
          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5
          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6
          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION
          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing
          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-19 02:13:14.619335) Review request for zookeeper and Benjamin Reed. Changes ------- Added javadocs Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs (updated) src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/276//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/276//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/276//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/12479713/ZOOKEEPER-965.patch against trunk revision 1103811. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/276//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/276//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/276//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          i'm almost done with the review. (it's a big patch!!!) there is a high level issue i'd like to bring up: i don't think the check request should return anything because if the check succeeds, you already know the information that is returned. (it's what you expected.) if the check fails, you just get an error code. if check doesn't return anything, we can either drop it if it succeeds in PrepRequestProcesor or propogate an error. this simplifies the rest of the pipeline a bit because they will not have to care about the check op.

          Show
          Benjamin Reed added a comment - i'm almost done with the review. (it's a big patch!!!) there is a high level issue i'd like to bring up: i don't think the check request should return anything because if the check succeeds, you already know the information that is returned. (it's what you expected.) if the check fails, you just get an error code. if check doesn't return anything, we can either drop it if it succeeds in PrepRequestProcesor or propogate an error. this simplifies the rest of the pipeline a bit because they will not have to care about the check op.
          Hide
          Marshall McMullen added a comment -

          Benjamin,

          I completely agree. At my company our internal code that uses zookeeper and the new multi op support does exactly what you suggest. We didn't see any need for the return from a check op, so we ignore it completely. I think your suggestion is a good one.

          Show
          Marshall McMullen added a comment - Benjamin, I completely agree. At my company our internal code that uses zookeeper and the new multi op support does exactly what you suggest. We didn't see any need for the return from a check op, so we ignore it completely. I think your suggestion is a good one.
          Hide
          Ted Dunning added a comment -

          Well, I am not entirely sure about ignoring the check result.

          First, the order and count of results is very handy to make sure that you can attach results back to which operation caused the result. I realize that there is no conditionality here but one-for-one correspondence in the results is very nice. The idea that multi transforms ops into results is nice and simple.

          It is true that it is hard to imagine that the Stat structure that comes back has any new information in it. I would be happy if the client inserts place-holder results. My original motivation was to design check as if it were a call to an exists method. As you point out, the client probably have done that already.

          Would you be happy with the client side insertion only? This would give all of the economies you want, but would preserve a simpler facade for the user.

          Show
          Ted Dunning added a comment - Well, I am not entirely sure about ignoring the check result. First, the order and count of results is very handy to make sure that you can attach results back to which operation caused the result. I realize that there is no conditionality here but one-for-one correspondence in the results is very nice. The idea that multi transforms ops into results is nice and simple. It is true that it is hard to imagine that the Stat structure that comes back has any new information in it. I would be happy if the client inserts place-holder results. My original motivation was to design check as if it were a call to an exists method. As you point out, the client probably have done that already. Would you be happy with the client side insertion only? This would give all of the economies you want, but would preserve a simpler facade for the user.
          Hide
          Marshall McMullen added a comment -

          Ted brings up a very valid point. All of the code in the server and both Java and C clients always assume the one-to-one correspondence between the number of ops submitted and the number of results.

          But, I'm not sure it needs to be a Stat that we put into the results. It could just have a placeholder or boolean value (which would always been one on a successful multi-op).

          Ted, I'm not sure I understand what you mean by client-side insertion only. Can you elaborate on what that would entail? Also, I'd be hesitant to do anything that makes the server-side (or the C client) processing any more complex or difficult than it already is....

          Show
          Marshall McMullen added a comment - Ted brings up a very valid point. All of the code in the server and both Java and C clients always assume the one-to-one correspondence between the number of ops submitted and the number of results. But, I'm not sure it needs to be a Stat that we put into the results. It could just have a placeholder or boolean value (which would always been one on a successful multi-op). Ted, I'm not sure I understand what you mean by client-side insertion only. Can you elaborate on what that would entail? Also, I'd be hesitant to do anything that makes the server-side (or the C client) processing any more complex or difficult than it already is....
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/#review689
          -----------------------------------------------------------

          src/c/include/zookeeper.h
          <https://reviews.apache.org/r/739/#comment1380>

          shouldn't this be a union

          src/c/include/zookeeper.h
          <https://reviews.apache.org/r/739/#comment1381>

          it isn't clear to me that we should expose check outside of a multitransaction.

          src/java/main/org/apache/zookeeper/Transaction.java
          <https://reviews.apache.org/r/739/#comment1382>

          should we also have an asynchronous version?

          src/java/main/org/apache/zookeeper/ZooKeeper.java
          <https://reviews.apache.org/r/739/#comment1383>

          i think we also need an asynchronous version.

          src/java/main/org/apache/zookeeper/server/DataTree.java
          <https://reviews.apache.org/r/739/#comment1384>

          if stat didn't return anything, we could skip this altogether. if we are going to return something, should we return more than the version?

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          <https://reviews.apache.org/r/739/#comment1385>

          i don't think we want to log this at all do we?

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          <https://reviews.apache.org/r/739/#comment1386>

          don't use tabs. spaces!

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          <https://reviews.apache.org/r/739/#comment1392>

          i think we are only half atomic here. if the multi txn fails half way through, it will not be applied to the database, but we will still have recorded it as pending, so we may fail later changes.

          for example, if there is a create for a znode that would otherwise succeed, but fails do to a later (in the multi txn) check failing. there will be a pending change record created, so even though the create failed, later creates to that znode will result in a node exists error.

          src/zookeeper.jute
          <https://reviews.apache.org/r/739/#comment1379>

          i don't think we should use TxnHeader since all we need is the type.

          • Benjamin

          On 2011-05-19 02:13:14, Ted Dunning wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/739/

          -----------------------------------------------------------

          (Updated 2011-05-19 02:13:14)

          Review request for zookeeper and Benjamin Reed.

          Summary

          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to

          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.

          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs

          -----

          src/c/Makefile.am 65830fe

          src/c/include/proto.h 843032f

          src/c/include/zookeeper.h c055edb

          src/c/src/zookeeper.c db715b0

          src/c/tests/TestMulti.cc PRE-CREATION

          src/c/tests/TestOperations.cc f9441ea

          src/c/tests/ZKMocks.cc a75dce6

          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION

          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION

          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION

          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f

          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012

          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e

          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d

          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2

          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff

          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929

          src/java/main/org/apache/zookeeper/server/package.html 3ec7656

          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5

          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6

          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION

          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing

          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/#review689 ----------------------------------------------------------- src/c/include/zookeeper.h < https://reviews.apache.org/r/739/#comment1380 > shouldn't this be a union src/c/include/zookeeper.h < https://reviews.apache.org/r/739/#comment1381 > it isn't clear to me that we should expose check outside of a multitransaction. src/java/main/org/apache/zookeeper/Transaction.java < https://reviews.apache.org/r/739/#comment1382 > should we also have an asynchronous version? src/java/main/org/apache/zookeeper/ZooKeeper.java < https://reviews.apache.org/r/739/#comment1383 > i think we also need an asynchronous version. src/java/main/org/apache/zookeeper/server/DataTree.java < https://reviews.apache.org/r/739/#comment1384 > if stat didn't return anything, we could skip this altogether. if we are going to return something, should we return more than the version? src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java < https://reviews.apache.org/r/739/#comment1385 > i don't think we want to log this at all do we? src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java < https://reviews.apache.org/r/739/#comment1386 > don't use tabs. spaces! src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java < https://reviews.apache.org/r/739/#comment1392 > i think we are only half atomic here. if the multi txn fails half way through, it will not be applied to the database, but we will still have recorded it as pending, so we may fail later changes. for example, if there is a create for a znode that would otherwise succeed, but fails do to a later (in the multi txn) check failing. there will be a pending change record created, so even though the create failed, later creates to that znode will result in a node exists error. src/zookeeper.jute < https://reviews.apache.org/r/739/#comment1379 > i don't think we should use TxnHeader since all we need is the type. Benjamin On 2011-05-19 02:13:14, Ted Dunning wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-19 02:13:14) Review request for zookeeper and Benjamin Reed. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs ----- src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Benjamin Reed added a comment -

          yeah, i was thinking along the same lines as you two: a boolean place holder would work out nicely.

          Show
          Benjamin Reed added a comment - yeah, i was thinking along the same lines as you two: a boolean place holder would work out nicely.
          Hide
          Ted Dunning added a comment -

          Ted, I'm not sure I understand what you mean by client-side insertion only. Can you elaborate on what that would entail? Also, I'd be hesitant to do anything that makes the server-side (or the C client) processing any more complex or difficult than it already is....

          What I meant was that the rules for inserting place-holder results are pretty simple. As such, if it is preferable to not return results from the server in these spots, then a simple merge could be used to create a results list with place-holder structures. Something like this:

          Iterator<Result> i = results.iterator();
          List<Result> r = Lists.newHashList();
          for (op in ops) {
            if (op.type() == CHECK) {
              r.add(PLACE_HOLDER);
            } else {
              r.add(i.next());
            }
          }
          

          I agree that complication of the client is bad, but this isn't much and if it makes the server side easier, then it isn't a big deal. My guess is that the server side solution is very simple (as in, just quit setting the version). Or the cost of getting the version may be low enough we really don't care.

          Show
          Ted Dunning added a comment - Ted, I'm not sure I understand what you mean by client-side insertion only. Can you elaborate on what that would entail? Also, I'd be hesitant to do anything that makes the server-side (or the C client) processing any more complex or difficult than it already is.... What I meant was that the rules for inserting place-holder results are pretty simple. As such, if it is preferable to not return results from the server in these spots, then a simple merge could be used to create a results list with place-holder structures. Something like this: Iterator<Result> i = results.iterator(); List<Result> r = Lists.newHashList(); for (op in ops) { if (op.type() == CHECK) { r.add(PLACE_HOLDER); } else { r.add(i.next()); } } I agree that complication of the client is bad, but this isn't much and if it makes the server side easier, then it isn't a big deal. My guess is that the server side solution is very simple (as in, just quit setting the version). Or the cost of getting the version may be low enough we really don't care.
          Hide
          Benjamin Reed added a comment -

          i think propagating a boolean from preprequestprocessor should work well. the main thing i wanted to avoid is adding the processing logic in the datatree.

          Show
          Benjamin Reed added a comment - i think propagating a boolean from preprequestprocessor should work well. the main thing i wanted to avoid is adding the processing logic in the datatree.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/#review706
          -----------------------------------------------------------

          src/java/main/org/apache/zookeeper/ZooKeeper.java
          <https://reviews.apache.org/r/739/#comment1410>

          What would you say about deferring this to a separate JIRA?

          • Ted

          On 2011-05-19 02:13:14, Ted Dunning wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/739/

          -----------------------------------------------------------

          (Updated 2011-05-19 02:13:14)

          Review request for zookeeper and Benjamin Reed.

          Summary

          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to

          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.

          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs

          -----

          src/c/Makefile.am 65830fe

          src/c/include/proto.h 843032f

          src/c/include/zookeeper.h c055edb

          src/c/src/zookeeper.c db715b0

          src/c/tests/TestMulti.cc PRE-CREATION

          src/c/tests/TestOperations.cc f9441ea

          src/c/tests/ZKMocks.cc a75dce6

          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION

          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION

          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION

          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f

          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012

          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e

          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d

          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2

          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff

          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929

          src/java/main/org/apache/zookeeper/server/package.html 3ec7656

          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5

          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6

          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION

          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing

          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/#review706 ----------------------------------------------------------- src/java/main/org/apache/zookeeper/ZooKeeper.java < https://reviews.apache.org/r/739/#comment1410 > What would you say about deferring this to a separate JIRA? Ted On 2011-05-19 02:13:14, Ted Dunning wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-19 02:13:14) Review request for zookeeper and Benjamin Reed. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs ----- src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Benjamin Reed added a comment -

          i think deferring the async version to a separate jira is reasonable since it is hard to keep the code fresh. we should open the jira before we commit the change though.

          Show
          Benjamin Reed added a comment - i think deferring the async version to a separate jira is reasonable since it is hard to keep the code fresh. we should open the jira before we commit the change though.
          Hide
          Ted Dunning added a comment -

          See ZOOKEEPER-1066 for the asynchronous version of multi.

          (I should file a JIRA for my inability to type asynchronous in less than 5 attempts as well)

          Show
          Ted Dunning added a comment - See ZOOKEEPER-1066 for the asynchronous version of multi. (I should file a JIRA for my inability to type asynchronous in less than 5 attempts as well)
          Hide
          Marshall McMullen added a comment -

          Hey Ted, I wanted to reply to some of the comments Benjamin made on review
          board, but I can't figure out how to do so. There's no place that I see to
          add comments or to login to the interface... should I just post my comments
          on Jira along with the original bug?

          Thanks...

          Show
          Marshall McMullen added a comment - Hey Ted, I wanted to reply to some of the comments Benjamin made on review board, but I can't figure out how to do so. There's no place that I see to add comments or to login to the interface... should I just post my comments on Jira along with the original bug? Thanks...
          Hide
          Patrick Hunt added a comment -

          Hi Marshall, when you open this link https://reviews.apache.org/r/739/ do you see a "login" and "register" links in the top right? (I do in chrome incognito window) If this is your first time on apache rb then you'll need to register.

          Show
          Patrick Hunt added a comment - Hi Marshall, when you open this link https://reviews.apache.org/r/739/ do you see a "login" and "register" links in the top right? (I do in chrome incognito window) If this is your first time on apache rb then you'll need to register.
          Hide
          Ted Dunning added a comment -

          Marshall,

          After logging in as Patrick says, you also need to click on the number on the far left. That will
          pop up an overlay window that will let you type in your response. Or riposte as the case may be.

          If the replies are general rather than line by line, you can also just put them here. That can
          even be better.

          Show
          Ted Dunning added a comment - Marshall, After logging in as Patrick says, you also need to click on the number on the far left. That will pop up an overlay window that will let you type in your response. Or riposte as the case may be. If the replies are general rather than line by line, you can also just put them here. That can even be better.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/c/include/zookeeper.h, line 1119

          > <https://reviews.apache.org/r/739/diff/4/?file=19416#file19416line1119>

          >

          > it isn't clear to me that we should expose check outside of a multitransaction.

          That's a valid point. I did it for completeness, but there's really no need to have a check op outside a multi op. It would clean up the code as well if I removed it. I'll try to get that done tonight as well.

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/java/main/org/apache/zookeeper/Transaction.java, line 57

          > <https://reviews.apache.org/r/739/diff/4/?file=19425#file19425line57>

          >

          > should we also have an asynchronous version?

          Agreed. Ted's created a new jira case for this.

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/java/main/org/apache/zookeeper/ZooKeeper.java, line 801

          > <https://reviews.apache.org/r/739/diff/4/?file=19427#file19427line801>

          >

          > i think we also need an asynchronous version.

          Agreed. Ted's created a new jira case for this.

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/java/main/org/apache/zookeeper/server/DataTree.java, line 817

          > <https://reviews.apache.org/r/739/diff/4/?file=19428#file19428line817>

          >

          > if stat didn't return anything, we could skip this altogether. if we are going to return something, should we return more than the version?

          I'm not sure I understand... can you elaborate ?

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 483

          > <https://reviews.apache.org/r/739/diff/4/?file=19430#file19430line483>

          >

          > i don't think we want to log this at all do we?

          Agreed. I'll clean that up tonight.

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 484

          > <https://reviews.apache.org/r/739/diff/4/?file=19430#file19430line484>

          >

          > don't use tabs. spaces!

          Sorry... at my day job we use tabs (I know) and I forgot to switch my settings around – I'm usually very careful about that .

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 489

          > <https://reviews.apache.org/r/739/diff/4/?file=19430#file19430line489>

          >

          > i think we are only half atomic here. if the multi txn fails half way through, it will not be applied to the database, but we will still have recorded it as pending, so we may fail later changes.

          >

          > for example, if there is a create for a znode that would otherwise succeed, but fails do to a later (in the multi txn) check failing. there will be a pending change record created, so even though the create failed, later creates to that znode will result in a node exists error.

          I thought not applying it to the database was sufficient, but perhaps I'm wrong. I thought the pending change records would get discarded if the multi op failed. Do I need to do that explicitly somehow? What are your thoughts on how to deal with this?

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/zookeeper.jute, line 267

          > <https://reviews.apache.org/r/739/diff/4/?file=19440#file19440line267>

          >

          > i don't think we should use TxnHeader since all we need is the type.

          Agreed. I'll see about refactoring that.

          On 2011-05-20 04:15:05, Benjamin Reed wrote:

          > src/c/include/zookeeper.h, line 273

          > <https://reviews.apache.org/r/739/diff/4/?file=19416#file19416line273>

          >

          > shouldn't this be a union

          It originally was a union but I ran into some annoyances when trying to use the C API from C++ code as C++ won't let you use designated initializers unless you compile with a newer --std flag (which, when enabled, caused other zookeeper code not to compile from C++). e.g. you get warnings like:

          tests/TestMulti.cc:689: warning: extended initializer lists only available with -std=c+0x or -std=gnu+0x

          Anyhow, I think the current implementation is a bit of a hack, and would prefer a union as well. Let me take another look at this tonight.

          • Marshall

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/#review689
          -----------------------------------------------------------

          On 2011-05-19 02:13:14, Ted Dunning wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/739/

          -----------------------------------------------------------

          (Updated 2011-05-19 02:13:14)

          Review request for zookeeper and Benjamin Reed.

          Summary

          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to

          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.

          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs

          -----

          src/c/Makefile.am 65830fe

          src/c/include/proto.h 843032f

          src/c/include/zookeeper.h c055edb

          src/c/src/zookeeper.c db715b0

          src/c/tests/TestMulti.cc PRE-CREATION

          src/c/tests/TestOperations.cc f9441ea

          src/c/tests/ZKMocks.cc a75dce6

          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION

          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION

          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION

          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f

          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012

          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e

          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d

          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2

          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff

          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929

          src/java/main/org/apache/zookeeper/server/package.html 3ec7656

          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5

          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6

          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION

          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing

          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/c/include/zookeeper.h, line 1119 > < https://reviews.apache.org/r/739/diff/4/?file=19416#file19416line1119 > > > it isn't clear to me that we should expose check outside of a multitransaction. That's a valid point. I did it for completeness, but there's really no need to have a check op outside a multi op. It would clean up the code as well if I removed it. I'll try to get that done tonight as well. On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/java/main/org/apache/zookeeper/Transaction.java, line 57 > < https://reviews.apache.org/r/739/diff/4/?file=19425#file19425line57 > > > should we also have an asynchronous version? Agreed. Ted's created a new jira case for this. On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/java/main/org/apache/zookeeper/ZooKeeper.java, line 801 > < https://reviews.apache.org/r/739/diff/4/?file=19427#file19427line801 > > > i think we also need an asynchronous version. Agreed. Ted's created a new jira case for this. On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/java/main/org/apache/zookeeper/server/DataTree.java, line 817 > < https://reviews.apache.org/r/739/diff/4/?file=19428#file19428line817 > > > if stat didn't return anything, we could skip this altogether. if we are going to return something, should we return more than the version? I'm not sure I understand... can you elaborate ? On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 483 > < https://reviews.apache.org/r/739/diff/4/?file=19430#file19430line483 > > > i don't think we want to log this at all do we? Agreed. I'll clean that up tonight. On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 484 > < https://reviews.apache.org/r/739/diff/4/?file=19430#file19430line484 > > > don't use tabs. spaces! Sorry... at my day job we use tabs (I know) and I forgot to switch my settings around – I'm usually very careful about that . On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 489 > < https://reviews.apache.org/r/739/diff/4/?file=19430#file19430line489 > > > i think we are only half atomic here. if the multi txn fails half way through, it will not be applied to the database, but we will still have recorded it as pending, so we may fail later changes. > > for example, if there is a create for a znode that would otherwise succeed, but fails do to a later (in the multi txn) check failing. there will be a pending change record created, so even though the create failed, later creates to that znode will result in a node exists error. I thought not applying it to the database was sufficient, but perhaps I'm wrong. I thought the pending change records would get discarded if the multi op failed. Do I need to do that explicitly somehow? What are your thoughts on how to deal with this? On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/zookeeper.jute, line 267 > < https://reviews.apache.org/r/739/diff/4/?file=19440#file19440line267 > > > i don't think we should use TxnHeader since all we need is the type. Agreed. I'll see about refactoring that. On 2011-05-20 04:15:05, Benjamin Reed wrote: > src/c/include/zookeeper.h, line 273 > < https://reviews.apache.org/r/739/diff/4/?file=19416#file19416line273 > > > shouldn't this be a union It originally was a union but I ran into some annoyances when trying to use the C API from C++ code as C++ won't let you use designated initializers unless you compile with a newer --std flag (which, when enabled, caused other zookeeper code not to compile from C++). e.g. you get warnings like: tests/TestMulti.cc:689: warning: extended initializer lists only available with -std=c+ 0x or -std=gnu +0x Anyhow, I think the current implementation is a bit of a hack, and would prefer a union as well. Let me take another look at this tonight. Marshall ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/#review689 ----------------------------------------------------------- On 2011-05-19 02:13:14, Ted Dunning wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-19 02:13:14) Review request for zookeeper and Benjamin Reed. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs ----- src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Marshall McMullen added a comment -

          Thanks Patrick, Ted. Don't know how I missed that the first time 'round... too little sleep I suppose .

          I've commented Benjamin's review. I'm also going to check in some updated changes to github to fix the things he requested fixing.

          Show
          Marshall McMullen added a comment - Thanks Patrick, Ted. Don't know how I missed that the first time 'round... too little sleep I suppose . I've commented Benjamin's review. I'm also going to check in some updated changes to github to fix the things he requested fixing.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-05-21 02:00:22, Ted Dunning wrote:

          > src/java/main/org/apache/zookeeper/ZooKeeper.java, line 801

          > <https://reviews.apache.org/r/739/diff/4/?file=19427#file19427line801>

          >

          > What would you say about deferring this to a separate JIRA?

          Does the separate JIRA you created address both of Benjamin's async requests? I think he wanted an async multi method as well as an async commit method...?

          • Marshall

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/#review706
          -----------------------------------------------------------

          On 2011-05-19 02:13:14, Ted Dunning wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/739/

          -----------------------------------------------------------

          (Updated 2011-05-19 02:13:14)

          Review request for zookeeper and Benjamin Reed.

          Summary

          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to

          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.

          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs

          -----

          src/c/Makefile.am 65830fe

          src/c/include/proto.h 843032f

          src/c/include/zookeeper.h c055edb

          src/c/src/zookeeper.c db715b0

          src/c/tests/TestMulti.cc PRE-CREATION

          src/c/tests/TestOperations.cc f9441ea

          src/c/tests/ZKMocks.cc a75dce6

          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION

          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION

          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION

          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f

          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012

          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e

          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d

          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2

          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff

          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929

          src/java/main/org/apache/zookeeper/server/package.html 3ec7656

          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5

          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6

          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION

          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing

          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-05-21 02:00:22, Ted Dunning wrote: > src/java/main/org/apache/zookeeper/ZooKeeper.java, line 801 > < https://reviews.apache.org/r/739/diff/4/?file=19427#file19427line801 > > > What would you say about deferring this to a separate JIRA? Does the separate JIRA you created address both of Benjamin's async requests? I think he wanted an async multi method as well as an async commit method...? Marshall ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/#review706 ----------------------------------------------------------- On 2011-05-19 02:13:14, Ted Dunning wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-19 02:13:14) Review request for zookeeper and Benjamin Reed. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs ----- src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Marshall McMullen added a comment -

          I've just pushed a change to github with the changes requested by Benjamin in his review on review board. It addresses these core issues:

          (1) Tighten up C API to use a union of structs for each op type instead of one mismashed set of fields. Removed macros to init zoo_op_t and created more explicit init functions for each type of op.

          (2) Removed zoo_check and zoo_acheck as there's no need to expose the check operation outside the context of a multi op. It was only added for completeness but it's totally unnecessaryR and needless adds clutter.

          (3) Removed superfluous logging in PrepRequestProcessor indicating ops were being skipped after a failing op in a multi op. There's already logging when the op fails, and a very clear log statement indicating all subsequent ops in the multi op will be skipped.

          (4) Converted all tabs to spaces (in my day job we use tabs and I forgot to adjust my editor's settings)

          (5) Removed TxnHeader from Txn since all we really need is the type. Replaced it with one int for the type.

          Show
          Marshall McMullen added a comment - I've just pushed a change to github with the changes requested by Benjamin in his review on review board. It addresses these core issues: (1) Tighten up C API to use a union of structs for each op type instead of one mismashed set of fields. Removed macros to init zoo_op_t and created more explicit init functions for each type of op. (2) Removed zoo_check and zoo_acheck as there's no need to expose the check operation outside the context of a multi op. It was only added for completeness but it's totally unnecessaryR and needless adds clutter. (3) Removed superfluous logging in PrepRequestProcessor indicating ops were being skipped after a failing op in a multi op. There's already logging when the op fails, and a very clear log statement indicating all subsequent ops in the multi op will be skipped. (4) Converted all tabs to spaces (in my day job we use tabs and I forgot to adjust my editor's settings) (5) Removed TxnHeader from Txn since all we really need is the type. Replaced it with one int for the type.
          Hide
          Benjamin Reed added a comment -

          excellent. wrt the pending ops in the prerequestprocessor. we have to undo them if we abort the mult-update. otherwise, the pending ops can cause future ops to incorrectly fail. (for example, if there was a pending create in a multi-update that is aborted, future creates will fail because the pending op data structure says that there is a create in process.) we have to update both the list and hashmap of pending operations. we also need to be careful since the pending operations are modified by the thread executing finalrequestprocessor.

          Show
          Benjamin Reed added a comment - excellent. wrt the pending ops in the prerequestprocessor. we have to undo them if we abort the mult-update. otherwise, the pending ops can cause future ops to incorrectly fail. (for example, if there was a pending create in a multi-update that is aborted, future creates will fail because the pending op data structure says that there is a create in process.) we have to update both the list and hashmap of pending operations. we also need to be careful since the pending operations are modified by the thread executing finalrequestprocessor.
          Hide
          Marshall McMullen added a comment -

          I've just checked in another change to Ted's github branch that fixes a nasty bug in the C API that we discovered while integrating the code into our code base. I think this is a critical fix to the C API that should absolutely be part of the final version we commit.

          From my commit message:

          ZOOKEEPER-965 - fix inoperable multi called from watch context.

          While integrating the new zoo_multi and zoo_amulti into the
          code at our company, we discovered a nasty bug in the C API. If
          a multi op is triggered from a watch callback, then it would hang
          indefinitely. This was due to a fundamental flaw in my earlier
          implementation whereby I treated all the ops in a multi op as
          asynchronous, with a forced synchronous tail completion that I
          tacked onto the end of my completion list. BUT, that tail completion
          ultimately never got called when called from a watch context!!

          The fix was ultimately to create code in both process functions
          (both sync and async) for dealing with multi-ops. Each op inside
          the multiop is dealt with the same, so they call into a common
          function. This way, all the existing logic for dealing with
          sync/async completions now deals with multi ops instead of
          me trying to deal with that specially.

          I've also added a new unit test that demonstrated this bug and
          also now passes with my refactoring.

          Show
          Marshall McMullen added a comment - I've just checked in another change to Ted's github branch that fixes a nasty bug in the C API that we discovered while integrating the code into our code base. I think this is a critical fix to the C API that should absolutely be part of the final version we commit. From my commit message: ZOOKEEPER-965 - fix inoperable multi called from watch context. While integrating the new zoo_multi and zoo_amulti into the code at our company, we discovered a nasty bug in the C API. If a multi op is triggered from a watch callback, then it would hang indefinitely. This was due to a fundamental flaw in my earlier implementation whereby I treated all the ops in a multi op as asynchronous, with a forced synchronous tail completion that I tacked onto the end of my completion list. BUT, that tail completion ultimately never got called when called from a watch context!! The fix was ultimately to create code in both process functions (both sync and async) for dealing with multi-ops. Each op inside the multiop is dealt with the same, so they call into a common function. This way, all the existing logic for dealing with sync/async completions now deals with multi ops instead of me trying to deal with that specially. I've also added a new unit test that demonstrated this bug and also now passes with my refactoring.
          Hide
          Ted Dunning added a comment -

          Updated patch with Marshall's latest.

          Show
          Ted Dunning added a comment - Updated patch with Marshall's latest.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12480807 ]
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/
          -----------------------------------------------------------

          (Updated 2011-05-29 23:54:13.115005)

          Review request for zookeeper and Benjamin Reed.

          Changes
          -------

          This should have fixes for a problem in the c api and for removing the pending changes in the data tree
          as suggested by Ben.

          Summary
          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to
          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.
          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs (updated)


          src/c/Makefile.am 65830fe
          src/c/include/proto.h 843032f
          src/c/include/zookeeper.h c055edb
          src/c/src/zookeeper.c db715b0
          src/c/tests/TestMulti.cc PRE-CREATION
          src/c/tests/TestOperations.cc f9441ea
          src/c/tests/ZKMocks.cc a75dce6
          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION
          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION
          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION
          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f
          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012
          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e
          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7
          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d
          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2
          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff
          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929
          src/java/main/org/apache/zookeeper/server/package.html 3ec7656
          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5
          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6
          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION
          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing
          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-29 23:54:13.115005) Review request for zookeeper and Benjamin Reed. Changes ------- This should have fixes for a problem in the c api and for removing the pending changes in the data tree as suggested by Ben. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs (updated) src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Hadoop QA added a comment -

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

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

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

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

          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/296//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/12480807/ZOOKEEPER-965.patch against trunk revision 1125581. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/296//console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          patch fails to apply against latest trunk (see console output)

          Show
          Patrick Hunt added a comment - patch fails to apply against latest trunk (see console output)
          Patrick Hunt made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Ted Dunning added a comment -

          Updated to track trunk

          Show
          Ted Dunning added a comment - Updated to track trunk
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12480874 ]
          Ted Dunning made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Ted Dunning made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/297//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/297//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/297//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/12480874/ZOOKEEPER-965.patch against trunk revision 1125581. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 48 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/297//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/297//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/297//console This message is automatically generated.
          Hide
          Ted Dunning added a comment -

          Rebased patch to current trunk. Note that there were several conflicts in the merge. The most important was in

          src/java/main/org/apache/zookeeper/server/DataTree.java around line 891.

          The conflict was versus changes introduced by ZOOKEEPER-1046

          Camille, if you can, could you check that my merge preserved your fix?

          Show
          Ted Dunning added a comment - Rebased patch to current trunk. Note that there were several conflicts in the merge. The most important was in src/java/main/org/apache/zookeeper/server/DataTree.java around line 891. The conflict was versus changes introduced by ZOOKEEPER-1046 Camille, if you can, could you check that my merge preserved your fix?
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12480882 ]
          Ted Dunning made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/298//testReport/
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/298//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/12480882/ZOOKEEPER-965.patch against trunk revision 1125581. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/298//testReport/ Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/298//console This message is automatically generated.
          Hide
          Ted Dunning added a comment -

          Cut and paste error in resolving conflict.

          Show
          Ted Dunning added a comment - Cut and paste error in resolving conflict.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12480886 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/299//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/299//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/299//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/12480886/ZOOKEEPER-965.patch against trunk revision 1125581. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/299//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/299//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-ZOOKEEPER-Build/299//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          reviewing the patch now.

          Show
          Benjamin Reed added a comment - reviewing the patch now.
          Hide
          Camille Fournier added a comment -

          Ted, looking at just the change in DataTree, I don't see any corresponding changes in FileTxnSnapLog. Is it possible to hit the same bug we saw in ZOOKEEPER-1046 in this multi case? I haven't reasoned it through yet but my instinct says yes, which means that FileTxnSnapLog needs to be changed to do the appropriate updating of cversion inside the components of the multi txn when the NONODE/NODEEXIST error conditions occur inside the multitxn.

          Other than that the merge itself looks ok I think.

          Show
          Camille Fournier added a comment - Ted, looking at just the change in DataTree, I don't see any corresponding changes in FileTxnSnapLog. Is it possible to hit the same bug we saw in ZOOKEEPER-1046 in this multi case? I haven't reasoned it through yet but my instinct says yes, which means that FileTxnSnapLog needs to be changed to do the appropriate updating of cversion inside the components of the multi txn when the NONODE/NODEEXIST error conditions occur inside the multitxn. Other than that the merge itself looks ok I think.
          Hide
          Ted Dunning added a comment -

          I don't see any corresponding changes in FileTxnSnapLog

          Of course, you are correct here. The individual operations in a multi work exactly like the
          individual operations. Unless we somehow inherit the right behavior, your change will need
          to be replicated.

          I won't get to this for a day or two. Would it be possible for you to drop this change in on
          the github version?

          Show
          Ted Dunning added a comment - I don't see any corresponding changes in FileTxnSnapLog Of course, you are correct here. The individual operations in a multi work exactly like the individual operations. Unless we somehow inherit the right behavior, your change will need to be replicated. I won't get to this for a day or two. Would it be possible for you to drop this change in on the github version?
          Hide
          Camille Fournier added a comment -

          Not before Wed PM but I can look at it then if you need.

          Show
          Camille Fournier added a comment - Not before Wed PM but I can look at it then if you need.
          Hide
          Ted Dunning added a comment -

          Wrote up an intro to the multi command. http://tdunning.blogspot.com/2011/06/tour-of-multi-update-for-zookeeper.html

          This references a simple multi sample application at github. https://github.com/tdunning/zookeeper

          Show
          Ted Dunning added a comment - Wrote up an intro to the multi command. http://tdunning.blogspot.com/2011/06/tour-of-multi-update-for-zookeeper.html This references a simple multi sample application at github. https://github.com/tdunning/zookeeper
          Hide
          Marshall McMullen added a comment -

          Ted, very nice write-up. I think it does a good job of explaining the problem and the solution. Nice job.

          Show
          Marshall McMullen added a comment - Ted, very nice write-up. I think it does a good job of explaining the problem and the solution. Nice job.
          Hide
          Marshall McMullen added a comment -

          So I'm finally getting a chance to circle back around to Benjamin's comments above about the stale ChangeRecords that get left in PrepRequestProcessor on a failed multi-op. I want to be sure I understand what needs to be done here, so Benjamin, feel free to correct me if I'm off track here....

          You mentioned:

          > we have to update both the list and hashmap of pending operations.

          If I'm reading this right, this is all currently updated via addChangeRecord (in PrepRequestProcess). I think it's adding it to the list via zks.outstandingChanges.add(c) and adding to the hashmap via zks.outstandingChangesForPath.put(c.path, c). So, adding in a removeChangeRecord method here that mirrors addChangeRecord looks like a good approach. I think I can use the transaction id on the failed multi op to identify all the failed change requests that need to be removed. Luckily, all change requests associated with a multiop will have the same txn id, so that should be super helpful here.

          Also, you mentioned :

          > we also need to be careful since the pending operations are modified by the thread executing Finalrequestprocessor

          Can you elaborate on what the concern is there? If I make the removeChangeRecord synchronized like addChangeRecord is, will that be sufficient?

          Show
          Marshall McMullen added a comment - So I'm finally getting a chance to circle back around to Benjamin's comments above about the stale ChangeRecords that get left in PrepRequestProcessor on a failed multi-op. I want to be sure I understand what needs to be done here, so Benjamin, feel free to correct me if I'm off track here.... You mentioned: > we have to update both the list and hashmap of pending operations. If I'm reading this right, this is all currently updated via addChangeRecord (in PrepRequestProcess). I think it's adding it to the list via zks.outstandingChanges.add(c) and adding to the hashmap via zks.outstandingChangesForPath.put(c.path, c). So, adding in a removeChangeRecord method here that mirrors addChangeRecord looks like a good approach. I think I can use the transaction id on the failed multi op to identify all the failed change requests that need to be removed. Luckily, all change requests associated with a multiop will have the same txn id, so that should be super helpful here. Also, you mentioned : > we also need to be careful since the pending operations are modified by the thread executing Finalrequestprocessor Can you elaborate on what the concern is there? If I make the removeChangeRecord synchronized like addChangeRecord is, will that be sufficient?
          Hide
          Marshall McMullen added a comment -

          OK, so I think this is actually pretty straightforward. I've written up an initial version to remove the failed op change records. I'm going to try to write a unit test that exhibits this theoretical bug and then try my fix and see if it fixes it.

          Show
          Marshall McMullen added a comment - OK, so I think this is actually pretty straightforward. I've written up an initial version to remove the failed op change records. I'm going to try to write a unit test that exhibits this theoretical bug and then try my fix and see if it fixes it.
          Hide
          Marshall McMullen added a comment -

          I am unable to create a unit test that fails due to the "half-atomic" bug Benjamin cited. It looks to me like even though there are stale change records for a failed multi op, they don't seem to cause any noticeable bug. I tried to do a multi that created a node, then tried to delete the same node twice. This leaves a change record for the create. Then I tried another subsequent multi to create that same node. That multi succeeds (b/c the actual create never succeeded and never made it to the database).

          I don't see any way this can actually cause a problem though I DO think it should be fixed to avoid any possible problems that I just can't think of a way to test. The fix is trivial anyhow. I iterate through the change records on a failed multi op and delete any that have the failed multi op's txn id.

          Anyhow, I'll commit this change to Ted's multi branch later today. If anyone can think of a unit test that fails without this fix, I'd love to see it.

          Thanks...

          Show
          Marshall McMullen added a comment - I am unable to create a unit test that fails due to the "half-atomic" bug Benjamin cited. It looks to me like even though there are stale change records for a failed multi op, they don't seem to cause any noticeable bug. I tried to do a multi that created a node, then tried to delete the same node twice. This leaves a change record for the create. Then I tried another subsequent multi to create that same node. That multi succeeds (b/c the actual create never succeeded and never made it to the database). I don't see any way this can actually cause a problem though I DO think it should be fixed to avoid any possible problems that I just can't think of a way to test. The fix is trivial anyhow. I iterate through the change records on a failed multi op and delete any that have the failed multi op's txn id. Anyhow, I'll commit this change to Ted's multi branch later today. If anyone can think of a unit test that fails without this fix, I'd love to see it. Thanks...
          Hide
          Marshall McMullen added a comment -

          OK, I've committed a change to delete the stale change records on a failed multiop. I've got a few other changes to make today that I'll push to the multi branch later today.

          Show
          Marshall McMullen added a comment - OK, I've committed a change to delete the stale change records on a failed multiop. I've got a few other changes to make today that I'll push to the multi branch later today.
          Hide
          Marshall McMullen added a comment -

          I'm now finishing up the other suggestion Benjamin had about using a boolean placeholder for a checkop instead of having a real Stat structure in there. This is also simpler than I was thinking so I should have this done shortly. Just testing it out now to ensure no regressions. And actually, as it turns out I don't even need a boolean at all – just a placeholder. B/c if the check failed, then we put a Error txn/response in the result array on the server side not a check response. Anyhow, should have this done in the next 30 minutes.

          Show
          Marshall McMullen added a comment - I'm now finishing up the other suggestion Benjamin had about using a boolean placeholder for a checkop instead of having a real Stat structure in there. This is also simpler than I was thinking so I should have this done shortly. Just testing it out now to ensure no regressions. And actually, as it turns out I don't even need a boolean at all – just a placeholder. B/c if the check failed, then we put a Error txn/response in the result array on the server side not a check response. Anyhow, should have this done in the next 30 minutes.
          Hide
          Marshall McMullen added a comment -

          I've just committed another change to Ted's multi branch to remove the Stat_t from a check op and use a simple placeholder as was discussed above.

          Show
          Marshall McMullen added a comment - I've just committed another change to Ted's multi branch to remove the Stat_t from a check op and use a simple placeholder as was discussed above.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/739/#review750
          -----------------------------------------------------------

          this looks really great! one thing to think about (you don't need to think about it too long, but it would be great if you could "fix it"): we generally only use generated jute Record instances. i think this makes it easier if we ever switch serialization libraries, but MultiResponse is not generated. i understand why, but it would be cool if we could use a generated class. this is not blocking the patch.

          the cleanup of pending ops was not in the uploaded diffs, but i looked at the code on git hub. i think this is almost right, but removeChangeRecord has a bug: we do the checking based on the Map not the list, so when we remove the ChangeRecord we need to check to see if there was something there before that we replaced and put it back. i think this is the last blocker that i can see.

          • Benjamin

          On 2011-05-29 23:54:13, Ted Dunning wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/739/

          -----------------------------------------------------------

          (Updated 2011-05-29 23:54:13)

          Review request for zookeeper and Benjamin Reed.

          Summary

          -------

          This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to

          succeed or fail together. Both C and java bindings are provided.

          This addresses bug ZOOKEEPER-965.

          https://issues.apache.org/jira/browse/ZOOKEEPER-965

          Diffs

          -----

          src/c/Makefile.am 65830fe

          src/c/include/proto.h 843032f

          src/c/include/zookeeper.h c055edb

          src/c/src/zookeeper.c db715b0

          src/c/tests/TestMulti.cc PRE-CREATION

          src/c/tests/TestOperations.cc f9441ea

          src/c/tests/ZKMocks.cc a75dce6

          src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION

          src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Op.java PRE-CREATION

          src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION

          src/java/main/org/apache/zookeeper/ZooDefs.java 832976f

          src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012

          src/java/main/org/apache/zookeeper/server/DataTree.java d16537e

          src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7

          src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d

          src/java/main/org/apache/zookeeper/server/Request.java a5c57e2

          src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff

          src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929

          src/java/main/org/apache/zookeeper/server/package.html 3ec7656

          src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5

          src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6

          src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION

          src/zookeeper.jute 7d96f32

          Diff: https://reviews.apache.org/r/739/diff

          Testing

          -------

          A number of unit tests have been implemented. Test coverage is very good.

          A sample application has been written to do simple operations on a graph of nodes.

          Thanks,

          Ted

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/#review750 ----------------------------------------------------------- this looks really great! one thing to think about (you don't need to think about it too long, but it would be great if you could "fix it"): we generally only use generated jute Record instances. i think this makes it easier if we ever switch serialization libraries, but MultiResponse is not generated. i understand why, but it would be cool if we could use a generated class. this is not blocking the patch. the cleanup of pending ops was not in the uploaded diffs, but i looked at the code on git hub. i think this is almost right, but removeChangeRecord has a bug: we do the checking based on the Map not the list, so when we remove the ChangeRecord we need to check to see if there was something there before that we replaced and put it back. i think this is the last blocker that i can see. Benjamin On 2011-05-29 23:54:13, Ted Dunning wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/739/ ----------------------------------------------------------- (Updated 2011-05-29 23:54:13) Review request for zookeeper and Benjamin Reed. Summary ------- This mega-patch adds the multi-op capability to ZK. This allows a batch of create, delete, update or version-check operations to succeed or fail together. Both C and java bindings are provided. This addresses bug ZOOKEEPER-965 . https://issues.apache.org/jira/browse/ZOOKEEPER-965 Diffs ----- src/c/Makefile.am 65830fe src/c/include/proto.h 843032f src/c/include/zookeeper.h c055edb src/c/src/zookeeper.c db715b0 src/c/tests/TestMulti.cc PRE-CREATION src/c/tests/TestOperations.cc f9441ea src/c/tests/ZKMocks.cc a75dce6 src/java/main/org/apache/zookeeper/MultiResponse.java PRE-CREATION src/java/main/org/apache/zookeeper/MultiTransactionRecord.java PRE-CREATION src/java/main/org/apache/zookeeper/Op.java PRE-CREATION src/java/main/org/apache/zookeeper/OpResult.java PRE-CREATION src/java/main/org/apache/zookeeper/Transaction.java PRE-CREATION src/java/main/org/apache/zookeeper/ZooDefs.java 832976f src/java/main/org/apache/zookeeper/ZooKeeper.java 00b4012 src/java/main/org/apache/zookeeper/server/DataTree.java d16537e src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 2538cf7 src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 50f208d src/java/main/org/apache/zookeeper/server/Request.java a5c57e2 src/java/main/org/apache/zookeeper/server/RequestProcessor.java 5c3e8ff src/java/main/org/apache/zookeeper/server/TraceFormatter.java 8ece929 src/java/main/org/apache/zookeeper/server/package.html 3ec7656 src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java 49affd5 src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java 0ad4dd6 src/java/test/org/apache/zookeeper/MultiResponseTest.java PRE-CREATION src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java PRE-CREATION src/zookeeper.jute 7d96f32 Diff: https://reviews.apache.org/r/739/diff Testing ------- A number of unit tests have been implemented. Test coverage is very good. A sample application has been written to do simple operations on a graph of nodes. Thanks, Ted
          Hide
          Marshall McMullen added a comment -

          Benjamin,

          Thanks for taking the time to look over my latest changes. I sort of get what you're suggesting, but I'm not sure about the implementation.

          Do you have any ideas in mind on how I'd "check to see if there was something there before that we replaced and put it back". I was thinking I could create a map of any existing change records and if the multi op fails, I can use that map to restore things to their proper state... Does that sound like what you were thinking?

          Show
          Marshall McMullen added a comment - Benjamin, Thanks for taking the time to look over my latest changes. I sort of get what you're suggesting, but I'm not sure about the implementation. Do you have any ideas in mind on how I'd "check to see if there was something there before that we replaced and put it back". I was thinking I could create a map of any existing change records and if the multi op fails, I can use that map to restore things to their proper state... Does that sound like what you were thinking?
          Hide
          Marshall McMullen added a comment -

          Benjamin,

          I've just pushed a change to github that implements what I think will work for the bug you cited in removeChangeRecords. Essentially, I store off prior change records (if any) for all the paths in the multi-op. Then if the multi-op fails, I restore the values in both the list zks.outstandingChanges and the hashmap zks.outstandingChangesForPath.

          Can you please look at this latest change and see if you see any problems with this latest approach? My only question is whether the order matters in the outstandingChanges list. Because I delete the old one in a for loop, then add the old (prior) one back onto the end of the list. I could deal with the order more carefully and put it back into the same index position if that's a requirement....

          Let me know what you think.

          Thanks!

          Show
          Marshall McMullen added a comment - Benjamin, I've just pushed a change to github that implements what I think will work for the bug you cited in removeChangeRecords. Essentially, I store off prior change records (if any) for all the paths in the multi-op. Then if the multi-op fails, I restore the values in both the list zks.outstandingChanges and the hashmap zks.outstandingChangesForPath. Can you please look at this latest change and see if you see any problems with this latest approach? My only question is whether the order matters in the outstandingChanges list. Because I delete the old one in a for loop, then add the old (prior) one back onto the end of the list. I could deal with the order more carefully and put it back into the same index position if that's a requirement.... Let me know what you think. Thanks!
          Hide
          Marshall McMullen added a comment -

          There were actually two commits to fix the above bug. So if you're just browsing the commits on github, make sure you look at both....

          Show
          Marshall McMullen added a comment - There were actually two commits to fix the above bug. So if you're just browsing the commits on github, make sure you look at both....
          Hide
          Benjamin Reed added a comment -

          no there may be a race condition. between the time you grab the old mapping and the time you abort, the old mapping may have been removed, so you don't want to put it back. you also don't need to re-add the prior change records to the outstandingchanges list.

          i think i have an efficient way of figuring out what to do:

          1) keep saving off the prior change records like you are doing now.

          if you abort, do the following synchronized with outstandingchanges
          2) start at the end of the list and work backwards removing elements from the end of the list as long as they are the zxid you are working on. once the zxid changes you can stop. (list will help you avoid going through the whole list.)
          3) look at the zxid of the first element of the outstandingchanges list (i'll call it firstZxid). go through your prior change records and remove any whose zxid < firstZxid.
          4) apply the remaining priorchange records to the outstandingchangesForPath.

          does that sound reasonable? 2) is kind of a pain because have to work with indices, but it shouldn't be too bad.

          Show
          Benjamin Reed added a comment - no there may be a race condition. between the time you grab the old mapping and the time you abort, the old mapping may have been removed, so you don't want to put it back. you also don't need to re-add the prior change records to the outstandingchanges list. i think i have an efficient way of figuring out what to do: 1) keep saving off the prior change records like you are doing now. if you abort, do the following synchronized with outstandingchanges 2) start at the end of the list and work backwards removing elements from the end of the list as long as they are the zxid you are working on. once the zxid changes you can stop. (list will help you avoid going through the whole list.) 3) look at the zxid of the first element of the outstandingchanges list (i'll call it firstZxid). go through your prior change records and remove any whose zxid < firstZxid. 4) apply the remaining priorchange records to the outstandingchangesForPath. does that sound reasonable? 2) is kind of a pain because have to work with indices, but it shouldn't be too bad.
          Hide
          Marshall McMullen added a comment -

          Benjamin, thanks for the quick reply. I think this sounds like a very easy thing to add to what I've done. Let me make sure I understand you though... Specifically:

          2) start at the end of the list and work backwards removing elements from the end of the list as long as they are the zxid you
          are working on. once the zxid changes you can stop. (list will help you avoid going through the whole list.)

          When you say "the zxid you are working on" do you mean the zxid for the multi op?

          Show
          Marshall McMullen added a comment - Benjamin, thanks for the quick reply. I think this sounds like a very easy thing to add to what I've done. Let me make sure I understand you though... Specifically: 2) start at the end of the list and work backwards removing elements from the end of the list as long as they are the zxid you are working on. once the zxid changes you can stop. (list will help you avoid going through the whole list.) When you say "the zxid you are working on" do you mean the zxid for the multi op?
          Hide
          Benjamin Reed added a comment -

          yes exactly, and that zxid will be at the end of the list since that is the zxid that you are currently working on.

          Show
          Benjamin Reed added a comment - yes exactly, and that zxid will be at the end of the list since that is the zxid that you are currently working on.
          Hide
          Marshall McMullen added a comment -

          Benjamin,

          I've just pushed a new change to github that I think does what you've suggested to solve the race condition you identified. Please look it over and let me know what you think.

          I haven't had a chance to look at using jute generated Records. I may have time to look at that tonight.

          Show
          Marshall McMullen added a comment - Benjamin, I've just pushed a new change to github that I think does what you've suggested to solve the race condition you identified. Please look it over and let me know what you think. I haven't had a chance to look at using jute generated Records. I may have time to look at that tonight.
          Hide
          Ted Dunning added a comment -

          The jute-ly issue can only be corrected by cut-and-paste coding on the jute
          definitions. Putting the message into jute will involve replicating a union
          of the fields in the sub-fields. With ZK's history of wire compatibility,
          that probably isn't a big deal.

          On Mon, Jun 6, 2011 at 10:56 AM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - The jute-ly issue can only be corrected by cut-and-paste coding on the jute definitions. Putting the message into jute will involve replicating a union of the fields in the sub-fields. With ZK's history of wire compatibility, that probably isn't a big deal. On Mon, Jun 6, 2011 at 10:56 AM, Marshall McMullen (JIRA)
          Hide
          Marshall McMullen added a comment -

          I'm not sure I feel comfortable altering the jute definitions themselves. Is this something you would be willing to tackle Ted? Otherwise perhaps we could defer this refactoring into a separate Jira case?

          Show
          Marshall McMullen added a comment - I'm not sure I feel comfortable altering the jute definitions themselves. Is this something you would be willing to tackle Ted? Otherwise perhaps we could defer this refactoring into a separate Jira case?
          Hide
          Ted Dunning added a comment -

          I would be happy to jump on this (just not right now).

          Deferring this is a fine thing, except that it is likely to be a wire-format
          change.

          Show
          Ted Dunning added a comment - I would be happy to jump on this (just not right now). Deferring this is a fine thing, except that it is likely to be a wire-format change.
          Hide
          Benjamin Reed added a comment -

          it's not a show stopper. it works fine with jute. we can fix it if it turns out to be a problem when we move to a real serialization api.

          Show
          Benjamin Reed added a comment - it's not a show stopper. it works fine with jute. we can fix it if it turns out to be a problem when we move to a real serialization api.
          Benjamin Reed made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Benjamin Reed added a comment -

          the code looks good. there is one other small thing. the case statement for multi-op is much to long. could you extract one or two sections of code into methods: grabPendingChanges() and rollbackPendingChanges() for example just to make it a bit smaller.

          once that is done we should be good to go. i'm cancelling the patch, so that we can be clear on the patch that you want loaded. (in other words, when you have a patch that you think should go in, submit it.)

          thanx for all your hard work!

          Show
          Benjamin Reed added a comment - the code looks good. there is one other small thing. the case statement for multi-op is much to long. could you extract one or two sections of code into methods: grabPendingChanges() and rollbackPendingChanges() for example just to make it a bit smaller. once that is done we should be good to go. i'm cancelling the patch, so that we can be clear on the patch that you want loaded. (in other words, when you have a patch that you think should go in, submit it.) thanx for all your hard work!
          Hide
          Marshall McMullen added a comment -

          Yes, I will do the refactor tonight.

          Thanks for looking it over Benjamin!

          Show
          Marshall McMullen added a comment - Yes, I will do the refactor tonight. Thanks for looking it over Benjamin!
          Hide
          Marshall McMullen added a comment -

          Benjamin,

          I've pushed a new set of changes to github with the multi-op error code refactored as you request. Let me know if this looks OK then we can proceed with generating a new patch....

          Thanks!

          Show
          Marshall McMullen added a comment - Benjamin, I've pushed a new set of changes to github with the multi-op error code refactored as you request. Let me know if this looks OK then we can proceed with generating a new patch.... Thanks!
          Hide
          Ted Dunning added a comment -

          I just integrated changes to trunk (ZOOKEEPER-1069) and rebased the github. This also picked up some small changes I made to the API to get results from the exception.

          Marshall, can you say if you are happy about the API change in 5ce6043f4 ?

          I have pushed the rebase to github so everybody will have to pull from scratch to make sure that they get the right history.

          I will also attach the patch for this right now.

          Show
          Ted Dunning added a comment - I just integrated changes to trunk ( ZOOKEEPER-1069 ) and rebased the github. This also picked up some small changes I made to the API to get results from the exception. Marshall, can you say if you are happy about the API change in 5ce6043f4 ? I have pushed the rebase to github so everybody will have to pull from scratch to make sure that they get the right history. I will also attach the patch for this right now.
          Hide
          Ted Dunning added a comment -

          I think this is the final state. Marshal should confirm.

          Show
          Ted Dunning added a comment - I think this is the final state. Marshal should confirm.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12481775 ]
          Hide
          Marshall McMullen added a comment -

          Ted, the changes in 5ce6043f4 look great to me. Much simpler than how I originally coded it. Thanks very much for refactoring that mess!

          Yes, I think this is the final state unless Benjamin sees anything amiss in my most recent commit to refactor the large multi-op case statement into a couple of functions.

          Otherwise I think we've finally finished this massive beast.

          Show
          Marshall McMullen added a comment - Ted, the changes in 5ce6043f4 look great to me. Much simpler than how I originally coded it. Thanks very much for refactoring that mess! Yes, I think this is the final state unless Benjamin sees anything amiss in my most recent commit to refactor the large multi-op case statement into a couple of functions. Otherwise I think we've finally finished this massive beast.
          Hide
          Marshall McMullen added a comment -

          Ted, can you post an updated patch?

          Show
          Marshall McMullen added a comment - Ted, can you post an updated patch?
          Hide
          Marshall McMullen added a comment -

          I downloaded the latest patch (08/Jun/11 05:55). It applies cleanly, but it does not compile:

          compile:
          [javac] Compiling 132 source files to /home/marshall/sandboxes/zookeeper-multi/build/classes
          [javac] /home/marshall/sandboxes/zookeeper-multi/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java:186: unreported exception java.io.IOException; must be caught or declared to be thrown
          [javac] rc = dt.processTxn(hdr, txn);
          [javac] ^
          [javac] /home/marshall/sandboxes/zookeeper-multi/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java:195: unreported exception java.io.IOException; must be caught or declared to be thrown
          [javac] rc = dt.processTxn(hdr, txn);
          [javac] ^
          [javac] /home/marshall/sandboxes/zookeeper-multi/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java:198: unreported exception java.io.IOException; must be caught or declared to be thrown
          [javac] rc = dt.processTxn(hdr, txn);
          [javac] ^
          [javac] 3 errors

          Can we get a new patch generated against latest trunk?

          Show
          Marshall McMullen added a comment - I downloaded the latest patch (08/Jun/11 05:55). It applies cleanly, but it does not compile: compile: [javac] Compiling 132 source files to /home/marshall/sandboxes/zookeeper-multi/build/classes [javac] /home/marshall/sandboxes/zookeeper-multi/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java:186: unreported exception java.io.IOException; must be caught or declared to be thrown [javac] rc = dt.processTxn(hdr, txn); [javac] ^ [javac] /home/marshall/sandboxes/zookeeper-multi/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java:195: unreported exception java.io.IOException; must be caught or declared to be thrown [javac] rc = dt.processTxn(hdr, txn); [javac] ^ [javac] /home/marshall/sandboxes/zookeeper-multi/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java:198: unreported exception java.io.IOException; must be caught or declared to be thrown [javac] rc = dt.processTxn(hdr, txn); [javac] ^ [javac] 3 errors Can we get a new patch generated against latest trunk?
          Hide
          Marshall McMullen added a comment -

          Ted, by the way I pulled the latest from your private git hub branch where this doesn't compile.

          Show
          Marshall McMullen added a comment - Ted, by the way I pulled the latest from your private git hub branch where this doesn't compile.
          Hide
          Marshall McMullen added a comment -

          I've pushed a new commit to github to resolve this. The bug was indeed inside the patch.

          Show
          Marshall McMullen added a comment - I've pushed a new commit to github to resolve this. The bug was indeed inside the patch.
          Hide
          Benjamin Reed added a comment -

          one other thing. we need to add a section on multi-update to the zookeeper programmers guide. it should be pretty quick just to add a multi-update section. feel free to open another jira to cover adding the doc so that we can get the code committed asap.

          i'd like to get this in soon.

          Show
          Benjamin Reed added a comment - one other thing. we need to add a section on multi-update to the zookeeper programmers guide. it should be pretty quick just to add a multi-update section. feel free to open another jira to cover adding the doc so that we can get the code committed asap. i'd like to get this in soon.
          Hide
          Marshall McMullen added a comment -

          Ted, is this something you can tackle? Also, did you look at the compilation failure I saw? I'd love to get this committed.

          Show
          Marshall McMullen added a comment - Ted, is this something you can tackle? Also, did you look at the compilation failure I saw? I'd love to get this committed.
          Hide
          Ted Dunning added a comment -

          I can take a quick look, but I am having trouble getting reliable net access.

          On Wed, Jun 15, 2011 at 6:29 AM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - I can take a quick look, but I am having trouble getting reliable net access. On Wed, Jun 15, 2011 at 6:29 AM, Marshall McMullen (JIRA)
          Hide
          Ted Dunning added a comment -

          OK. As a first step, I rebased our changes to current trunk.

          This will require the usual recheckout due to non-fast-forward operations.

          Now to the problems you are seeing.

          Show
          Ted Dunning added a comment - OK. As a first step, I rebased our changes to current trunk. This will require the usual recheckout due to non-fast-forward operations. Now to the problems you are seeing.
          Hide
          Ted Dunning added a comment -

          I see a clean compile on my mac. Looks like I don't understand the problem.
          I can't run all the tests just now, but last time I looked they ran.

          BuzzBook-Pro:zookeeper[trunk*]$ git checkout multi
          Switched to branch 'multi'
          BuzzBook-Pro:zookeeper[multi*]$ ant clean
          Buildfile: /Users/tdunning/Apache/zookeeper/build.xml
          ...
          clean:
          BUILD SUCCESSFUL
          Total time: 0 seconds
          BuzzBook-Pro:zookeeper[multi*]$ ant compile
          ...
          version-info:
          [java] Unknown REVISION number, using -1
          ...
          [javac] Compiling 52 source files to
          /Users/tdunning/Apache/zookeeper/build/classes
          ...
          [javac] Compiling 134 source files to
          /Users/tdunning/Apache/zookeeper/build/classes
          BUILD SUCCESSFUL
          Total time: 11 seconds
          BuzzBook-Pro:zookeeper[multi*]$

          On Wed, Jun 15, 2011 at 10:01 AM, Ted Dunning (JIRA) <jira@apache.org>
          https://issues.apache.org/jira/browse/ZOOKEEPER-965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13049670#comment-13049670]
          ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
          ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
          ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
          ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
          ZOOKEEPER-965.patch, ZOOKEEPER-965.patch, ZOOKEEPER-965.patch,
          ZOOKEEPER-965.patch, ZOOKEEPER-965.patch
          a list of create, delete, update or check objects each of which has a
          desired version or file state in the case of create. If all of the version
          and existence constraints can be satisfied, then all updates will be done
          atomically.
          other style has a "Transaction" that allows builder-like methods to build a
          set of updates and a commit method to finalize the transaction. This can
          trivially be reduced to the first kind of API so the list based API style
          should be considered the primitive and the builder style should be
          implemented as syntactic sugar.
          transaction should be limited to 1MB.
          internals. The changes include:
          form, the code should be slightly extended to convert a list of operations
          to idempotent form.
          be detected gracefully and an informative exception should be thrown.
          at https://github.com/tdunning/zookeeper and am happy to extend committer
          status to anyone who agrees to donate their code back to Apache. The final
          patch will be attached to this bug as normal.

          Show
          Ted Dunning added a comment - I see a clean compile on my mac. Looks like I don't understand the problem. I can't run all the tests just now, but last time I looked they ran. BuzzBook-Pro:zookeeper [trunk*] $ git checkout multi Switched to branch 'multi' BuzzBook-Pro:zookeeper [multi*] $ ant clean Buildfile: /Users/tdunning/Apache/zookeeper/build.xml ... clean: BUILD SUCCESSFUL Total time: 0 seconds BuzzBook-Pro:zookeeper [multi*] $ ant compile ... version-info: [java] Unknown REVISION number, using -1 ... [javac] Compiling 52 source files to /Users/tdunning/Apache/zookeeper/build/classes ... [javac] Compiling 134 source files to /Users/tdunning/Apache/zookeeper/build/classes BUILD SUCCESSFUL Total time: 11 seconds BuzzBook-Pro:zookeeper [multi*] $ On Wed, Jun 15, 2011 at 10:01 AM, Ted Dunning (JIRA) <jira@apache.org> https://issues.apache.org/jira/browse/ZOOKEEPER-965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13049670#comment-13049670 ] ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch, ZOOKEEPER-965 .patch a list of create, delete, update or check objects each of which has a desired version or file state in the case of create. If all of the version and existence constraints can be satisfied, then all updates will be done atomically. other style has a "Transaction" that allows builder-like methods to build a set of updates and a commit method to finalize the transaction. This can trivially be reduced to the first kind of API so the list based API style should be considered the primitive and the builder style should be implemented as syntactic sugar. transaction should be limited to 1MB. internals. The changes include: form, the code should be slightly extended to convert a list of operations to idempotent form. be detected gracefully and an informative exception should be thrown. at https://github.com/tdunning/zookeeper and am happy to extend committer status to anyone who agrees to donate their code back to Apache. The final patch will be attached to this bug as normal.
          Hide
          Marshall McMullen added a comment -

          Ted, thanks for taking a look at this. Not sure if you noticed this, but I checked in a change to github to fix the compile error I was seeing. I just wanted you to look at it and see if/why the fix was necessary....

          Show
          Marshall McMullen added a comment - Ted, thanks for taking a look at this. Not sure if you noticed this, but I checked in a change to github to fix the compile error I was seeing. I just wanted you to look at it and see if/why the fix was necessary....
          Hide
          Ted Dunning added a comment -

          Ahhh... no.

          I didn't notice that. I will take a look.

          On Wed, Jun 15, 2011 at 4:45 PM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - Ahhh... no. I didn't notice that. I will take a look. On Wed, Jun 15, 2011 at 4:45 PM, Marshall McMullen (JIRA)
          Hide
          Ted Dunning added a comment -

          Marshall,

          I just tried with and without your patch. It compiles either way.

          My feeling is that excessive throws declarations are bad juju anyway so the current state (with your change)
          is better than the previous state (with the extra throws in processTxn).

          I would leave it as is.

          Show
          Ted Dunning added a comment - Marshall, I just tried with and without your patch. It compiles either way. My feeling is that excessive throws declarations are bad juju anyway so the current state (with your change) is better than the previous state (with the extra throws in processTxn). I would leave it as is.
          Hide
          Marshall McMullen added a comment -

          I agree completely. Thanks for looking it over Ted. Do we need an updated patch to capture my latest change then?

          Also, do you want to create a new jira case for the changes Benjamin wanted in the zookeeper programmer's guide so that doesn't hold up committing this change?

          Show
          Marshall McMullen added a comment - I agree completely. Thanks for looking it over Ted. Do we need an updated patch to capture my latest change then? Also, do you want to create a new jira case for the changes Benjamin wanted in the zookeeper programmer's guide so that doesn't hold up committing this change?
          Hide
          Benjamin Reed added a comment -

          do you guys have a patch ready for commit? mahadev is putting together the 3.4.0 release.

          Show
          Benjamin Reed added a comment - do you guys have a patch ready for commit? mahadev is putting together the 3.4.0 release.
          Hide
          Marshall McMullen added a comment -

          Ted, can you generate a patch? If not I will generate one as I definitely one this going into 3.4.0.

          Show
          Marshall McMullen added a comment - Ted, can you generate a patch? If not I will generate one as I definitely one this going into 3.4.0.
          Hide
          Ted Dunning added a comment -

          I will get it out this morning.

          Show
          Ted Dunning added a comment - I will get it out this morning.
          Hide
          Ted Dunning added a comment -

          Here is the latest patch rebased to latest trunk. It compiles cleanly and I am currently running tests.

          I will update github shortly.

          Show
          Ted Dunning added a comment - Here is the latest patch rebased to latest trunk. It compiles cleanly and I am currently running tests. I will update github shortly.
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12483314 ]
          Hide
          Ted Dunning added a comment -

          Pushed to github.

          Show
          Ted Dunning added a comment - Pushed to github.
          Hide
          Marshall McMullen added a comment -

          The only other outstanding item here was to open a Jira case for the zookeeper programmer's guide. Can you do that too Ted?

          Show
          Marshall McMullen added a comment - The only other outstanding item here was to open a Jira case for the zookeeper programmer's guide. Can you do that too Ted?
          Hide
          Ted Dunning added a comment -

          Done. See ZOOKEEPER-1102

          On Tue, Jun 21, 2011 at 7:49 PM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - Done. See ZOOKEEPER-1102 On Tue, Jun 21, 2011 at 7:49 PM, Marshall McMullen (JIRA)
          Hide
          Marshall McMullen added a comment -

          Awesome! Thanks so much for quickly taking care of these items Ted.

          Show
          Marshall McMullen added a comment - Awesome! Thanks so much for quickly taking care of these items Ted.
          Hide
          Ted Dunning added a comment -

          My tests completed successfully.

          On Tue, Jun 21, 2011 at 7:57 PM, Marshall McMullen (JIRA)

          Show
          Ted Dunning added a comment - My tests completed successfully. On Tue, Jun 21, 2011 at 7:57 PM, Marshall McMullen (JIRA)
          Hide
          Ted Dunning added a comment -

          Final patch for committing

          Show
          Ted Dunning added a comment - Final patch for committing
          Ted Dunning made changes -
          Attachment ZOOKEEPER-965.patch [ 12484833 ]
          Ted Dunning made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/363//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/363//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/363//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/12484833/ZOOKEEPER-965.patch against trunk revision 1140017. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/363//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/363//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/363//console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          Committed revision 1141746.

          thanx ted and marshall! good work.

          Show
          Benjamin Reed added a comment - Committed revision 1141746. thanx ted and marshall! good work.
          Benjamin Reed made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1230 (See https://builds.apache.org/job/ZooKeeper-trunk/1230/)
          ZOOKEEPER-965. Need a multi-update command to allow multiple znodes to be updated safely

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

          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooDefs.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/MultiTransactionRecord.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/package.html
          • /zookeeper/trunk/src/c/tests/ZKMocks.cc
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Transaction.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/KeeperException.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
          • /zookeeper/trunk/src/c/Makefile.am
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/MultiResponse.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java
          • /zookeeper/trunk/src/c/src/zookeeper.c
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/MultiResponseTest.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/Request.java
          • /zookeeper/trunk/src/zookeeper.jute
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/TraceFormatter.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java
          • /zookeeper/trunk/src/c/include/proto.h
          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/RequestProcessor.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/OpResult.java
          • /zookeeper/trunk/src/c/include/zookeeper.h
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java.orig
          • /zookeeper/trunk/src/c/tests/TestOperations.cc
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1230 (See https://builds.apache.org/job/ZooKeeper-trunk/1230/ ) ZOOKEEPER-965 . Need a multi-update command to allow multiple znodes to be updated safely breed : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1141746 Files : /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooDefs.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/MultiTransactionRecord.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/util/SerializeUtils.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/package.html /zookeeper/trunk/src/c/tests/ZKMocks.cc /zookeeper/trunk/src/java/main/org/apache/zookeeper/Transaction.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/KeeperException.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java /zookeeper/trunk/src/c/Makefile.am /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/MultiResponse.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java /zookeeper/trunk/src/c/src/zookeeper.c /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/DataTree.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/MultiResponseTest.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/Request.java /zookeeper/trunk/src/zookeeper.jute /zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/TraceFormatter.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/MultiTransactionRecordTest.java /zookeeper/trunk/src/c/include/proto.h /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/server/RequestProcessor.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/OpResult.java /zookeeper/trunk/src/c/include/zookeeper.h /zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/CnxManagerTest.java.orig /zookeeper/trunk/src/c/tests/TestOperations.cc
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1232 (See https://builds.apache.org/job/ZooKeeper-trunk/1232/)
          ZOOKEEPER-965. Need a multi-update command to allow multiple znodes to be updated safely
          (missed a file)

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

          • /zookeeper/trunk/src/c/tests/TestMulti.cc
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1232 (See https://builds.apache.org/job/ZooKeeper-trunk/1232/ ) ZOOKEEPER-965 . Need a multi-update command to allow multiple znodes to be updated safely (missed a file) breed : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1142377 Files : /zookeeper/trunk/src/c/tests/TestMulti.cc
          Ted Dunning made changes -
          Link This issue is related to ZOOKEEPER-1124 [ ZOOKEEPER-1124 ]
          Patrick Hunt made changes -
          Issue Type Bug [ 1 ] New Feature [ 2 ]
          Mahadev konar made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Ted Dunning
              Reporter:
              Ted Dunning
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development