> 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?
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? - Ivan ----------------------------------------------------------- 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 > >
