> 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.
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. - 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 > >
