> 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 > >
