> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > > > Sijie Guo wrote: > ok. let me step back. how bad is the current protocol? if no, why we keep > arguing on this? as those changes will break our system, then I don't see the > value of contributing our patches back to the community. > > Ivan Kelly wrote: > For me, the motivating change was auth. The initial drop I did for > auth[1] was based in PB to allow for auth providers to include their own > extensions with PB and not have to deal with parsing. This was mixing old > parsing and PB though, so I held off the change until PB went in, so that we > wouldnt have two parsing mechanisms being actively developed. > > For me, this motivation is still valid. Having two decoding mechanisms is > ugly and hacky. With the plan to add batch reads,it also makes sense to be > easily able to add messages. > > Another motivation is to reduce the variation between the branches. We > currently have 3 branchs, the apache branch, the twitter branch and the yahoo > branch (perhaps huawei also have a branch). These will unlikely ever be 100% > identical, but I had hoped that we could keep the difference to a minimum to > allow for us to be able to benefit from each others changes. > > That said, changes from all sides need to go through the full review > process before going in. Both companies have different production usecases, > roadmaps, and rollout mechanisms, so assumptions may not hold across > organisations. > > To bring it back to the current patch, and your initial question, the > current protocol isn't terrible. The code for it is a hell of a lot cleaner > than it was 2 years ago. However, it's cumbersome to add anything to the > protocol, mostly because of decisions taken early on, (such as the difference > in position of ledgerid and entryid in read and add requests). I was hoping > we could avoid locking ourselves into something like this again. > > [1] https://github.com/ivankelly/bookkeeper/commits/BookKeeperAuth > > Sijie Guo wrote: > ok. replies on your individual concerns. > > Ivan Kelly wrote: > We seem to be going round in circles here. I've replied below. But to > compromise on somethings, if you change the flags and only have double > statuses for responses that need them (i.e. add an intermediate message), I > can live with enum status codes, operationType and a whole lot of stuff being > required when it should be optional. If you make those changes, Ill push the > commit. Otherwise, we need to bring in a tiebreaker.
sure. I also replied all your comments. if I missed anyone, let me know. > 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? 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? - 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 > >
