----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review37545 -----------------------------------------------------------
Ship it! This is a great, I just have a few minor points and clarifications below. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java <https://reviews.apache.org/r/17895/#comment69348> Just to confirm, this is an improvement unrelated to this issue, yes? bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java <https://reviews.apache.org/r/17895/#comment69349> 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. bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java <https://reviews.apache.org/r/17895/#comment69139> I think we still need to add the license header, no? bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java <https://reviews.apache.org/r/17895/#comment69345> Should we create a jira for this todo? bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java <https://reviews.apache.org/r/17895/#comment69346> This comment is repeated from the previous file. Do we need a jira for this todo? bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java <https://reviews.apache.org/r/17895/#comment69347> "... running in read-only mode..." bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java <https://reviews.apache.org/r/17895/#comment69350> 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. - fpj On Feb. 10, 2014, 7:11 a.m., Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17895/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2014, 7:11 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 > 962f3a3 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java > 241fdbb > > 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 > 0757f87 > > 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 8404e3f > > 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 > >
