> On July 25, 2012, 5:27 a.m., Sijie Guo wrote: > > Thanks Rakesh for providing the patch. > > > > I had two questions about the ReadStrategy. > > > > 1) DistributionSchedule indicates how we store entries, which is bound with > > a ledger. so it is bound with ledger. it would be recorded in ledger > > metadata in future, if we started providing a different schedule not store > > entries in round-robin way. so when we open a ledger, we know how the > > ledger store entries. but ReadStrategy provides how we read entries from a > > ledger. so a kind of DistributionSchedule could have several different read > > strategies, user could specify the read strategy for a kind of distribution > > schedule. > > > > for example, currently the distribution schedule is RoundRobin. user could > > configure different read strategy as 'roundrobinReadStrategy'. so you just > > need to implement getReadStrategy in RoundRobinDistributionSchedule, and it > > would read setting 'roundrobinReadStrategy' to know which read strategy > > used for read entries in RoundRobinDistributionSchedule. > > > > so the two read strategies could be SimpleRoundRobinReadStrategy and > > PriorityRoundRobinReadStrategy. > > > > 2) It would be better to let ReadStrategy provide simple interface as below. > > > > public void readEntry(LedgerHandle lh, long entryId, > > GenericCallback<LedgerEntry> callback); > > > > the method would return LedgerEntry for a specific entry id, so all the > > retry logic could be hide under the interface. LedgerEntry could be defined > > as interface. For SimpleRoundRobinReadStrategy, it return a simple ledger > > entry which still uses nextReplicationIndex to select next bookie to retry. > > > > the method would decouple the logic reading several entries and reading > > specific entry in PendingReadOp. PendingReadOp just handle the logic to > > read several entries, while ReadStrategy handle the logic to read specific > > entry (handle retry logic). I think the changes in PendingReadOp is minor. > > one is to implement GenericCallback<LedgerEntry> callback instead of > > ReadEntryCallback, the other one is we don't put LedgerEntry in > > ArrayDequeue when initialOp, we could set it in > > GenericCallback<LedgerEntry>.
Thanks Sijie for your time and the detailed info. >>>>>1) DistributionSchedule indicates how we store entries, which is bound >>>>>with a ledger As you described, ReadStrategy will be maintained separately outside DistributionSchedule interface and ReadStrategy will be injected to the DistributionSchedule as follows: RoundRobinDistributionSchedule implements DistributionSchedule,ReadStrategy -> Will be difficult to enforce the readStrategy to the schedule algorithm as it is a separate interface and not part of the DistributionSchedule? -> Also, I think SimpleRoundRobinReadStrategy, PriorityRoundRobinReadStrategy are completely biased with RR. But I'm considering this as a separate read strategy applicable to any distribution scheduling algorithm. If agrees, I'll remove the dependency of ReadStrategy with DistributionSchedule and will keep as a separate instance under LedgerHandle.java. How does it sound? -> Also, in that case we should expose total two configuration items for the defining scheduling algo like: distributionScheduleAlgo = roundRobin (defining the scheduling algo) readStrategy = orderedRoundRobin (defining the reading strategy) >>>>>2) It would be better to let ReadStrategy provide simple interface as >>>>>below. Yeah its good, I'll do the changes according to this. > On July 25, 2012, 5:27 a.m., Sijie Guo wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 103 > > <https://reviews.apache.org/r/6128/diff/1/?file=128580#file128580line103> > > > > Why not let ClientConfiguration returns DistributionSchedule directly? Will it be good to instantiate strategies under the config object? > On July 25, 2012, 5:27 a.m., Sijie Guo wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedReadStrategy.java, > > line 39 > > <https://reviews.apache.org/r/6128/diff/1/?file=128581#file128581line39> > > > > since it tried to read entry from good bookies to bad bookies, I think > > it is a kind of priority read. A better name is PriorityReadStrategy. Fine will do respective changes > On July 25, 2012, 5:27 a.m., Sijie Guo wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedReadStrategy.java, > > line 45 > > <https://reviews.apache.org/r/6128/diff/1/?file=128581#file128581line45> > > > > ReadStrategy is part of DistributionSchedule, it would be instantiated > > per ledger. so these info just be used in this ledger. But the statistics > > could be leveraged across all ledgers opened by a client. My idea is to let > > the bookie statistics managed by a separated module per client. And the > > *OrderedReadStrategy* could used it to determine its reading order. Hope your idea is to keep the statistics under BookKeeper client, so that all the read operations under that client will have the intelligence. > On July 25, 2012, 5:27 a.m., Sijie Guo wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedRoundRobinDistributionSchedule.java, > > line 31 > > <https://reviews.apache.org/r/6128/diff/1/?file=128582#file128582line31> > > > > it is not a good idea to have it as a static variable and return it > > thru a class method. Will move to BookKeeper client > On July 25, 2012, 5:27 a.m., Sijie Guo wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java, > > line 34 > > <https://reviews.apache.org/r/6128/diff/1/?file=128585#file128585line34> > > > > it is not a good idea to have it as a static variable and return it > > thru a class method. Will move to BookKeeper client - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6128/#review9430 ----------------------------------------------------------- On July 25, 2012, 2:15 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6128/ > ----------------------------------------------------------- > > (Updated July 25, 2012, 2:15 a.m.) > > > Review request for bookkeeper, fpj and Ivan Kelly. > > > Description > ------- > > Improve read Perf: > bookie readEntries is taking more time if the ensemble has failed bookie(s) > > > This addresses bug BOOKKEEPER-336. > http://issues.apache.org/jira/browse/BOOKKEEPER-336 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DistributionSchedule.java > 1365007 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java > 1365007 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java > 1365007 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedReadStrategy.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedRoundRobinDistributionSchedule.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java > 1365007 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadStrategy.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java > 1365007 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/SimpleReadStrategy.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java > 1365007 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieReadLedgerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/6128/diff/ > > > Testing > ------- > > Patch contains test cases > > > Thanks, > > Rakesh R > >
