----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6128/#review9430 -----------------------------------------------------------
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>. http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/6128/#comment20188> Why not let ClientConfiguration returns DistributionSchedule directly? http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedReadStrategy.java <https://reviews.apache.org/r/6128/#comment20172> 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. http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedReadStrategy.java <https://reviews.apache.org/r/6128/#comment20177> 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. http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedRoundRobinDistributionSchedule.java <https://reviews.apache.org/r/6128/#comment20186> since it tried read entries from high priority (good one) bookies to low priority (bad one) bookies, how about renaming it as PriorityRoundRobinDistributionSchedule? http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/OrderedRoundRobinDistributionSchedule.java <https://reviews.apache.org/r/6128/#comment20179> it is not a good idea to have it as a static variable and return it thru a class method. http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RoundRobinDistributionSchedule.java <https://reviews.apache.org/r/6128/#comment20180> it is not a good idea to have it as a static variable and return it thru a class method. - Sijie Guo 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 > >
