> 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
ok. replies on your individual concerns. > On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33> > > > > This should certainly not be an enum. Otherwise we need to bump the > > protocol version each time we add an error code. > > > > Imagine the scenario where both server and client are running 4.3.0. > > Then the server is upgraded with 4.3.1 which a new error EPRINTERONFIRE. It > > sends this to the client who throws a decode error. > > Sijie Guo wrote: > how could being not enum help this? if it is integer, client still has no > idea how to interpret, so it is still invalid response from 4.3.0 client. I > thought we reached an agreement on enum on the ticket, no? > > Ivan Kelly wrote: > So for version and operationtype, enum is ok. These originate at the > client, so if the servers are always upgraded at the client, there's no > interoperability issues. Status codes originate at the server though, so it > is possible for the server to send a statuscode that is unrecognised to a > client. The normal way to handle this would be a "else" or "default:" to pass > this up to the client as a BKException.UnexpectedConditionException. If it's > an enum, this will throw a decode exception in the netty decoder, which is > harder to handle. > > To resolve this on the server side, by checking the version and only > sending errors valid for that version, implies two things. Firstly, every > error code change will require the version to be bumped and secondly, that > there will need to be a list maintained for which errors are valid for each > version. This goes against the motivation for using protobuf in the first > place. > > Sijie Guo wrote: > this is the application level agreement, no? it doesn't matter that u are > using a protobuf protocol or using current protocol, or it also doesn't > matter that u are using an integer or an enum. in any case, the best way is > as you described, you shouldn't send new status code back to an old client, > as the new status code is meaningless to the old client. > > Ivan Kelly wrote: > but how do you know its an old client? Only by bumping the version number > each time you add an error code. In which case you end up with a whole lot of > junk like "if (client.version == X) { send A } else if (client.version == Y) > { send B } else if (client.version ..." which is exactly what protobuf was > designed to avoid (see "A bit of history" on > https://developers.google.com/protocol-buffers/docs/overview). > > Sijie Guo wrote: > a else or default branch would make the behavior unpredictable as an old > client is treating a new status code as some kind of unknown. as you said, > you want to treat them as UnexpectedConditionException. But what does > UnexpectedConditionException means? doesn't it mean the sever already breaks > backward compatibility, since server couldn't satisfy the old client's > request. > > so still, if server wants to be backward compatibility to clients, in any > cases, it needs to know what version of protocol that the client is speaking > and handle them accordingly, not just let client to do their job in an > unexpected way. > > I don't see any elegant solutions without detecting protocol version. if > you have, please describe how not being enum would avoid this. > > Ivan Kelly wrote: > the default behaviour for an unknown error code is something we already > use today. > > https://github.com/apache/bookkeeper/blob/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L714 > > The client only needs to know that the request failed. the point of the > different error codes is so that the client could take specific recovery > steps. the default behaviour is just to pass the error up. the default behavior was there just for all already known status codes. but it doesn't mean it is correct for any unknown status codes. and when u are saying 'the client only needs to know that the request failed', you are making an assumption that there is only one status code indicating OK, other status code should be taken as failed. but it isn't true. say in an old protocol, we supported range reads, it responded with OK, list of entry response (0 = <data>, 1 = missing, 2 = missing, 3 = <data>). if we are going to improve our protocol to make communication more efficient, we are going to change the protocol to get rid of transferring missing entries: responding with PARTIAL_OK, list of existing entries (0 = <data>, 3 = <data>). in this case, if server doesn't distinguish the client's protocol, just respond every range reads with PARTIAL_OK, would did break the compatibility with old protocol, as old protocol treats it as failure by default behavior. in order to maintain backward compatibility, server needs to detect the client's protocol and responds accordingly. as what I said, for backward compatibility, a protocol doesn't really help if it involves behavior in application agreement. and a default behavior is not exact right. for me, using protocol. it means that it is easy for us to add new requests type, add new fields in existing requests. besides that, we need to keep the backward compatibility in our level. > 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. 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. > 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. 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. - 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 > >
