> 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. > > Sijie Guo wrote: > being Flag will make it easier to add other flags in future. > > Ivan Kelly wrote: > No it doesn't. Lets say you do add another flag in the future. How do you > specify that it's both a FENCE_LEDGER and NEW_FLAG? You need the field to be > repeated, and then you need code to loop through the flags. Compared to > msg.isFenceLedger(), this seems much more cumbersome. > > Sijie Guo wrote: > I don't say this field is bits-set-like of flags. as the long poll > changes we mentioned, we are going to add a new flag 'ENTRY_PIGGYBACK' for > read. so a read request is either a normal read (without Flag), a fence read > (flag = FENCE_LEDGER) or a piggyback read (flag = ENTRY_PIGGYBACK). Being > repeated will be kind of complicated, as you need to loop all possible > permutation. > > Ivan Kelly wrote: > This is ugly and shortsighted. There may well be a case in the future > where two flags need to be specified. What do we do then? Add a flags2 field? > > Sijie Guo wrote: > why this couldn't be a 3rd flag? FENCE_LEDGER_AND_PIGGYBACK? > > in your repeated way, let's say you have n flags, you would have n! ways > to combine them. it is hard to figure out what kind of combination works for > what situation. as some of your flags are exclusive with others. so renaming > the combination specifically would make it clear. > > Ivan Kelly wrote: > as you say, with n flags specified, there are n! combinations. so if > another flag as added, you will have to add n values to the enum to cover the > different combinations. whether the flags are exclusive is for the > application to decide. it shouldnt be implicit in the protocol encoding. > > Sijie Guo wrote: > same explanation for OperationType is applying here. if n flags mostly > are used individually, an explicit way will just need to deal with around n > possibilities. but you need to write lots of if..else branches for inferring > in an implicit way. > > Ivan Kelly wrote: > Your assumption here is that all the flag handling code will be in one > place in a big switch. this isnt the case. I dont know how you have > implemented piggyback, but i would guess it would be something like. > > Response processReadRequest(Request req) { > if (isFencing(req)) { > fenceLedger(); > } > byte[] data = fetchEntry(req.getLedgerId(), req.getEntryId()); > Response resp = new Response(OK, data); > if (isPiggyback(req)) { > addPiggyback(resp); > } > return resp; > } > > Now consider how #isPiggyback and #isFencing would be implemented with > enums and with individual boolean flags? which is shorter? Which is easier to > maintain? > > Sijie Guo wrote: > isFencing => flag == Flag.FENCE_LEDGER > isPiggyback => flag == ENTRY_PIGGYBACK > > what is the difference above for maintaining? > > using the flag, the semantic is super clear that your request is either a > fencing request or a piggyback request. it would not happen that you received > a request that probably have two flags enabled, then what does it mean if the > requests have two boolean flags enabled, don't consider the case (that is > always the point that I made for OperationType and Flag here)? then which is > easier to maintain? > > Ivan Kelly wrote: > my comment from yesterday doesn't wasnt saved by review board >:| > > Anyhow, what i asked was, can you guarantee that there will never *ever* > be a case where two flags will need to be set on a single op in the future? > > Sijie Guo wrote: > well, I already made a comment on your case: "why this couldn't be a 3rd > flag? FENCE_LEDGER_AND_PIGGYBACK?", no? and this is how hedwig handles > subscription mode, 'create', 'attach', 'create_and_attach', no? > > and again, as I said in previous comment, if you define flags in an > explicit way, you would have much clearer branches on what kind of operations > should be executed on what flags, like below: (take an example, A, B, C, > A_AND_B, B_AND_A) > > switch (flag) { > case A: > a(); > break; > case B: > b(); > break; > case C: > c(); > break; > case A_AND_B: > a(); > b(); > break; > case B_AND_A: > b(); > // other executions between b() and a(), which is different from > A_AND_B case. > a(); > break; > } > > but if you define flags in an implicit way, you need to check all > possible negative cases to avoid the program failed in abnormal cases (e.g. > bad combinations, bad order of flags). isn't that introducing much harder > work for maintenance? >
If we have a set of flags, also consider both AND/OR conjunctions the combinations may grow and thus increases the complexity of the problem. I feel it doesn't matter whether you are using enum type or bool flag, both will have 'chain of if conditions' either at the client side or at the server side. If I understood correctly the switch case you are talking will be added at the server side. Now for setting 'A_AND_B' enum type in the request, client side we need to have the 'chain of if conditions' no?. I could see few advantages of having 'if chain' at the client side, server would not worry about the conditional evaluation which may be in a critical section. Secondly it would be easy to define strategies against enum type and pass it over different layers without worrying about how they formed. If we think the problem space is small and assume we have only few set of flags I prefer enum. Before reaching to a conclusion I've one question about the number 'n' of flags and how much value 'n' can grow ?. Only my worry is, once we set the protocol to enum type, as we all know couldn't get another chance to change it later as 'n' increases. - Rakesh ----------------------------------------------------------- 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 > >
