> 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.
>
> Sijie Guo wrote:
> >> -> 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?
>
> Rakesh R wrote:
> >>>As my comment, one distribution schedule might have different read
> strategies.
> I got the idea.
> I'm just doubting, in future, will this duplicate the readstrategies in
> different scheduling algo?
>
> At present, read strategies will depends on the scheule for knowing the
> bookieIndex
> lh.distributionSchedule.getBookieIndex(entryId, replicaIndex++);
>
> Whats your thoughts?
>> in future, will this duplicate the readstrategies in different scheduling
>> algo?
If a read strategy could be applied in different scheduling algo, it could be
leveraged. we would not duplicate the read strategy implementation. only
duplicated might be configuration settings. I had no better idea about it.
Let's see other's comments.
- 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
>
>
