> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java, > > line 131 > > <https://reviews.apache.org/r/17895/diff/2/?file=563019#file563019line131> > > > > This change seems unrelated. I don't mind it going in, but could you > > give some background as to why it's here?
we want to specifically call out when read entry fails for ledger not existing. so in the application, some cases that handling for NoSuchLedgerExists and NoSuchEntryExists maybe identical, so we could let the caller decide how they want to differentiate these exceptions. > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java, > > line 129 > > <https://reviews.apache.org/r/17895/diff/2/?file=563024#file563024line129> > > > > typo will fix > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33> > > > > 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. how could being not enum help this? if it is integer, client still has no idea how to interpret, so it is still invalid response from 4.3.0 client. I thought we reached an agreement on enum on the ticket, no? > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 65 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line65> > > > > 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 > > > comment 1. OperationType will make request more clear and specify and less if...else branches. And OperationType is used for PCBC to unify all requests handling. so we don't need to have individual maps to maintain requests each time we added a new type of requests. > comment 2. if reply to comment 1 works for u, I don't see any reason why operation would not be a required. > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80> > > > > This would be better served with a boolean. protobuf will keep it > > compact. being Flag will make it easier to add other flags in future. > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 92 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line92> > > > > same as fenced, better as boolean same as previous comment. > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 104 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line104> > > > > You're sending StatusCode twice, every time. Firstly, shouldn't be an > > enum as stated above. Secondly, shouldn't be required. > enum as the comments above. > status code twice. the status code for ReadResponse/AddResponse is for individual responses. if we are going to support kind of batch reads, the status code of Response means either the whole batch succeed or not while the individual read responses might have different status codes (e.g OK or NotExists). That is why it exists two places. > required as the comments above - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review41130 ----------------------------------------------------------- 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 > >
