You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Rakesh R <ra...@huawei.com> on 2012/07/25 04:15:49 UTC
Review Request: Improve read Perf: bookie readEntries is taking more time if
ensemble has failed bookie(s)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6128/
-----------------------------------------------------------
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
Re: Review Request: Improve read Perf: bookie readEntries is taking more
time if ensemble has failed bookie(s)
Posted by Sijie Guo <gu...@gmail.com>.
> 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
>
>
Re: Review Request: Improve read Perf: bookie readEntries is taking more
time if ensemble has failed bookie(s)
Posted by Sijie Guo <gu...@gmail.com>.
> 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
>
>
Re: Review Request: Improve read Perf: bookie readEntries is taking more
time if ensemble has failed bookie(s)
Posted by Rakesh R <ra...@huawei.com>.
> 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
>
>
Re: Review Request: Improve read Perf: bookie readEntries is taking more
time if ensemble has failed bookie(s)
Posted by Rakesh R <ra...@huawei.com>.
> 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
>
>
Re: Review Request: Improve read Perf: bookie readEntries is taking more
time if ensemble has failed bookie(s)
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
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
>
>