> On March 19, 2014, 10:49 a.m., fpj wrote: > > This is a great, I just have a few minor points and clarifications below. > > Sijie Guo wrote: > Thanks Flavio for reviewing. Will address your comments.
sorry for late response. comments inline > On March 19, 2014, 10:49 a.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java, > > line 131 > > <https://reviews.apache.org/r/17895/diff/1/?file=481707#file481707line131> > > > > Just to confirm, this is an improvement unrelated to this issue, yes? yup. it is unrelated. but since I generated the patch by diffing our branch with trunk branch, so I didn't split them. > On March 19, 2014, 10:49 a.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java, > > line 41 > > <https://reviews.apache.org/r/17895/diff/1/?file=481710#file481710line41> > > > > It is good to use the version number (PreV3, V3) to differentiate parts > > of the code that serve different versions. This is a bit messy, though, > > because the version alone doesn't say what change actually triggered the > > code split. I was wondering if it makes sense to use ProtoBuf or PB, > > instead of V3, just to make it a bit more clear what change it is referring > > to in version 3. could I have a separated task to do renaming? > On March 19, 2014, 10:49 a.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java, > > line 1 > > <https://reviews.apache.org/r/17895/diff/1/?file=481713#file481713line1> > > > > I think we still need to add the license header, no? added > On March 19, 2014, 10:49 a.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java, > > line 69 > > <https://reviews.apache.org/r/17895/diff/1/?file=481718#file481718line69> > > > > This comment is repeated from the previous file. Do we need a jira for > > this todo? created https://issues.apache.org/jira/browse/BOOKKEEPER-748 > On March 19, 2014, 10:49 a.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java, > > line 51 > > <https://reviews.apache.org/r/17895/diff/1/?file=481719#file481719line51> > > > > "... running in read-only mode..." fixed > On March 19, 2014, 10:49 a.m., fpj wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java, > > line 530 > > <https://reviews.apache.org/r/17895/diff/1/?file=481726#file481726line530> > > > > Just to confirm, are we relying on the fact that compatibility with > > 4.1.0 implies compatibility with 4.2.0? I'm basically trying to understand > > why we don't need to add a new test case to test backward compatibility. there is no change on protocol between 4.1.0 and 4.2.0. so we could use 4.1.0 implies compatibility with 4.2.0. but if adding 4.2.0 would make it clear, I could create a jira for it. - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review37545 ----------------------------------------------------------- On April 21, 2014, 5:58 a.m., Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17895/ > ----------------------------------------------------------- > > (Updated April 21, 2014, 5:58 a.m.) > > > Review request for bookkeeper and Ivan Kelly. > > > Bugs: BOOKKEEPER-582 > https://issues.apache.org/jira/browse/BOOKKEEPER-582 > > > Repository: bookkeeper-git > > > Description > ------- > > - introducing protobuf support for bookkeeper > - for server: introduce packet processor / EnDecoder for different protocol > supports > - for client: change PCBC to use protobuf to send requests > - misc changes for protobuf support > > (bookie server is able for backward compatibility) > > > Diffs > ----- > > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexInMemPageMgr.java > 56487aa > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java > 28e23d6 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java > fb36b90 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/processor/RequestProcessor.java > 241f369 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java > 1154047 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestHandler.java > b922a82 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java > 8155b22 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBaseV3.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java > a10f7d5 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessorV3.java > PRE-CREATION > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto PRE-CREATION > bookkeeper-server/src/main/resources/findbugsExclude.xml 97a6156 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/TestProtoVersions.java > 5fcc445 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java > 3f8496f > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java > bc05229 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java > 8376b46 > > Diff: https://reviews.apache.org/r/17895/diff/ > > > Testing > ------- > > unit tests. backward tests. > > > Thanks, > > Sijie Guo > >
