> 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 > > > > Sijie Guo wrote: > > 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. > > Ivan Kelly wrote: > Ok. I can live with it, though I still think it's a little cumbersome. > Regarding requiredness, I still think it should be optional. All fields > should be optional by default to give you as much freedom to evolve the > protocol in the future as possible. Especially something, which is not 100% > essential to the protocol, like this field. > > Sijie Guo wrote: > for those fields that already exist in the origin protocol, I don't see > we are going to remove them. that's why we put it as required. > > Ivan Kelly wrote: > We shouldnt prelude the possibility though. What problem could making > them optional cause? > > Sijie Guo wrote: > what does an response w/o OperationType mean? How PCBC would handle this? > as I said, OperationType is somehow guiding us to dispatch the request and > response. so it is required. > > Ivan Kelly wrote: > the operation type can be inferred from the contents as i said already. > So OperationType itself isnt actually required, so we should lock it in at > this point. > > Sijie Guo wrote: > say you have A, B, C types of requests, you have resp_A, resp_B, resp_C > optional fields in response. if using OperationType, you explicitly know what > response that the request is for. it reduce the branches that u need to check > if the response is valid or not: for example, for response A, it just need to > check if resp_A exists or not. if not, it is an invalid response for A. you > don't need to touch resp_B and resp_C. > > if OperationType is not used, you need to write lots of if..else branches > to identify what's the response for. e.g. hasResp_A & !hasResp_B & !hasResp_C > is for request type A. even worser, if you are adding new request type, you > need to change all the if...else branches. > > Ivan Kelly wrote: > You dont need to check all negative cases. But thats beside the point. My > point is that it is conceivable to not have operationType, even though im ok > will it going through with this patch, and since it is conceivable for it to > be absent, we should leave open the possibility by making it optional.
sure for positive possibilities, we should leave them there for protocol evaluation. if this possibility will introduce negative effects, why not just avoid it? isn't a OperationType make things clear and robust, and much easier for maintenance? - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review41130 ----------------------------------------------------------- On April 24, 2014, 7:43 a.m., Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17895/ > ----------------------------------------------------------- > > (Updated April 24, 2014, 7:43 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/pom.xml ebc1198 > > 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 > compat-deps/bookkeeper-server-compat-4.2.0/pom.xml PRE-CREATION > compat-deps/hedwig-server-compat-4.2.0/pom.xml PRE-CREATION > compat-deps/pom.xml f79582d > hedwig-server/pom.xml 06cf01c > > hedwig-server/src/test/java/org/apache/hedwig/server/TestBackwardCompat.java > 8da109e > > Diff: https://reviews.apache.org/r/17895/diff/ > > > Testing > ------- > > unit tests. backward tests. > > > Thanks, > > Sijie Guo > >
