> 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>.
>
> Rakesh R wrote:
> 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.
>> -> Will be difficult to enforce the readStrategy to the schedule algorithm
>> as it is a separate interface and not part of the DistributionSchedule?
>> -> Also, in that case we should expose total two configuration items for the
>> defining scheduling algo like:
As my comment, one distribution schedule might have different read strategies.
A read strategy might not be used by several distribution schedules. I think
you could keep readStrategy in DistributionSchedule. But the read strategy
configuration setting is '{distributionScheduleName}ReadStrategy' which means
the read strategy used by a distribution schedule.
For roundrobin distribute schedule, its read strategies could be configured
thru 'roundrobinReadStrategy'.
>> distributionScheduleAlgo = roundRobin (defining the scheduling algo)
currently we just have round robin distribution schedule, we don't need to have
this setting now. we could have it after we had different distribution
schedules.
How does it sound?
> 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.
>
> Rakesh R wrote:
> Hope your idea is to keep the statistics under BookKeeper client, so that
> all the read operations under that client will have the intelligence.
yes. that's my idea.
- Sijie
-----------------------------------------------------------
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
>
>
