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