----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review41130 -----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java <https://reviews.apache.org/r/17895/#comment74543> This change seems unrelated. I don't mind it going in, but could you give some background as to why it's here? bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java <https://reviews.apache.org/r/17895/#comment74542> typo bookkeeper-server/src/main/proto/BookkeeperProtocol.proto <https://reviews.apache.org/r/17895/#comment74544> This should certainly not be an enum. Otherwise we need to bump the protocol version each time we add an error code. Imagine the scenario where both server and client are running 4.3.0. Then the server is upgraded with 4.3.1 which a new error EPRINTERONFIRE. It sends this to the client who throws a decode error. bookkeeper-server/src/main/proto/BookkeeperProtocol.proto <https://reviews.apache.org/r/17895/#comment74547> Comment 1. OperationType is redundant. The server can do #hasReadRequest and #hasAddRequest to check the request to check the type. We shouldn't send redundant data over the wire where possible. Comment 2. it better to make the absolute minimum to be required. Required is forever, and these may not be. https://developers.google.com/protocol-buffers/docs/proto bookkeeper-server/src/main/proto/BookkeeperProtocol.proto <https://reviews.apache.org/r/17895/#comment74545> This would be better served with a boolean. protobuf will keep it compact. bookkeeper-server/src/main/proto/BookkeeperProtocol.proto <https://reviews.apache.org/r/17895/#comment74546> same as fenced, better as boolean bookkeeper-server/src/main/proto/BookkeeperProtocol.proto <https://reviews.apache.org/r/17895/#comment74548> You're sending StatusCode twice, every time. Firstly, shouldn't be an enum as stated above. Secondly, shouldn't be required. - Ivan Kelly 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 > >
