On Tue, May 27, 2014 at 11:52 PM, Rakesh R <[email protected]> wrote:
> > > > 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? > > > > > > Rakesh R wrote: > > 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. > > > > Sijie Guo wrote: > > > Now for setting 'A_AND_B' enum type in the request, client side we > need to have the 'chain of if conditions' no?. > > > > why client need to have the chain? client just need to set different > flag value to indicate different type of single read. > > > > > how much value 'n' can grow? > > > > I could tell the future. but I don't see it would grow too much. A > specific enum value would make semantic more clear. > > I'm assuming there will be a conditional logic in order to set the flag > and the code will looks like, > > Flag flag = // init with default value > > if flag_condition_a satisfies > flag = A > if flag_condition_b satisfies > flag = B > if flag_condition_a satisfies && flag_condition_b satisfies > flag = A_AND_B > > > - Rakesh > sure. setting flag isn't a problem as you don't need to eliminate negative cases when adding more flags. but checking flags is, as you need to eliminate negative cases. > > > ----------------------------------------------------------- > 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 > > > > > >
