> On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 > > <https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81> > > > > ledgerId and entryId should be optional in all requests. It may be the > > case, that how we specify them changes in the future (like when we flatten > > the metadata), so it would be good to leave that possibility open. > > Sijie Guo wrote: > for all existing reads/writes protocol, the ledgerId and entryId are > required. I am not sure how u will change the protocol by flatten the > metadata. but I guess that will be pretty new protocol. if so, it would be > better to add new request type, so we will not break any existing systems or > making it being complicated. > > Ivan Kelly wrote: > Im not sure how it will change either. What I'm requesting is that the > protobuf protocol doesnt lock us in to how we are doing it now forever. > > Ivan Kelly wrote: > actually for a concrete example, lets say we want to do a read request > for a whole ledger, from bookie A we request all entries, but it doesnt have > every 3rd entry due to striping. In the case we can request all entrys from > the ledger with entryid modulo 3 from bookie B. In this case, what would I > put in the _required_ entry id firld for the read to bookie B? > > Sijie Guo wrote: > as I said, doesn't it sound like a new read protocol? > > Ivan Kelly wrote: > it will likely go through the same codepaths though, so we'll end up with > a load of duplicate code. my concern with the requiredness of fields is that > it's so rigid that in future we will have to add new messages to make any > enhancements, causing the protocol to grow into something huge, with loads of > redundancy, and not any better than we have now with the manual encoding. > > Sijie Guo wrote: > single entry read/write are primitives of bookie, ledger id and entry id > are required for them as they are the fundamental of bookkeeper. all other > improvements like streaming or range read could be built on these primitives. > then, if they are built on primitives, I don't see we will end up with a lot > of duplicated codes. > > Rakesh R wrote: > As far as compatibility issue is concerned, making optional is defensive > approach and safe coding by avoiding parsing issues later. But it should be > done very carefully at the code level because the requiredness is now > handling at the code level. For example, at the server it should do > validation to see a request has both ledgerId/entryId and which is mandatory > or not etc. On the otherside for the required field, I could see the entity > is not open for expansion by removing that field. But we have again options > like like Sijie suggested, by defining new protocol and do the expansion. > > If we have a better way in hand to avoid code duplication, it is OK to go > ahead with required. > > Sijie Guo wrote: > as I said, currently bookie storage is built on single read/add entry > primitives. there isn't any reason to make ledger id and entry id not to be > required. if you are going to change the protocol to get rid of ledger id and > entry id, you have to change the bookie storage. then I don't think there > will be any code duplication. > > Rakesh R wrote: > I agree this for add entry but while reading 'entryId' can be optional. > There is no real functional issues, only the concern is this will force us to > create new protocol if any such requirement comes in future. > > Sijie Guo wrote: > again, as I said current bookie storage is per entry. if you want to > support batch reads: 1) if you don't change bookie storage, you could build > the batch read protocol on top of single read primitive. 2) if you changed > bookie storage itself to support batch reads inside the storage, then it > should be new request type to use new method in bookie storage, so the old > read could still work with the storage that only support single read. this is > for backward compatible.
one more comment, for any protocol requirements, please remember what kind of operations supported now in bookie storage, and what's the backward compatibility on bookie storage. - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review41281 ----------------------------------------------------------- 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 > >
