You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Jiannan Wang <ji...@yahoo-inc.com> on 2012/11/13 05:53:12 UTC
Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger manager
for bookkeeper client.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/
-----------------------------------------------------------
Review request for bookkeeper.
Description
-------
Implement a MetaStore based ledger manager for bookkeeper client.
This addresses bug BOOKKEEPER-205.
https://issues.apache.org/jira/browse/BOOKKEEPER-205
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java e6a3807
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 2692fde
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 16c0276
bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java d528a18
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 2f39536
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
Diff: https://reviews.apache.org/r/8035/diff/
Testing
-------
Thanks,
Jiannan Wang
Re: Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger
manager for bookkeeper client.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/#review13974
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment29894>
Ah, you're right. I didn't see the subMap earlier.
- Ivan Kelly
On Nov. 13, 2012, 4:53 a.m., Jiannan Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8035/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 4:53 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> Implement a MetaStore based ledger manager for bookkeeper client.
>
>
> This addresses bug BOOKKEEPER-205.
> https://issues.apache.org/jira/browse/BOOKKEEPER-205
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java e6a3807
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 2692fde
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 16c0276
> bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java d528a18
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 2f39536
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
>
> Diff: https://reviews.apache.org/r/8035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiannan Wang
>
>
Re: Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger
manager for bookkeeper client.
Posted by Jiannan Wang <ji...@yahoo-inc.com>.
> On Dec. 3, 2012, 11:34 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 596
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line596>
> >
> > From ZooKeeper? Surely from metastore
Yeah, u r right.
> On Dec. 3, 2012, 11:34 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 618
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line618>
> >
> > Again, the zk reference is incorrect.
will change the name
> On Dec. 3, 2012, 11:34 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 576
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line576>
> >
> > There's a problem here. If you only read maxEntriesPerScan in each scan, then when you pass msActiveLedgers to doGC, it may not contain all active ledger ids (if there are more ledgers than maxEntriesPerScan). This will result in any ledger which were not in the scan, but which do exist, being gc'd.
We scan all the ledger ID in order, and at each round only maxEntriesPerScan are scanned (again, it's order), so I think the situation you described will not happen.
> On Dec. 3, 2012, 11:34 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 104
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line104>
> >
> > Create another JIRA for this TODO
> >
Will do it
- Jiannan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/#review13964
-----------------------------------------------------------
On Nov. 13, 2012, 4:53 a.m., Jiannan Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8035/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 4:53 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> Implement a MetaStore based ledger manager for bookkeeper client.
>
>
> This addresses bug BOOKKEEPER-205.
> https://issues.apache.org/jira/browse/BOOKKEEPER-205
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java e6a3807
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 2692fde
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 16c0276
> bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java d528a18
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 2f39536
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
>
> Diff: https://reviews.apache.org/r/8035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiannan Wang
>
>
Re: Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger
manager for bookkeeper client.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/#review13964
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment29872>
Create another JIRA for this TODO
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment29875>
There's a problem here. If you only read maxEntriesPerScan in each scan, then when you pass msActiveLedgers to doGC, it may not contain all active ledger ids (if there are more ledgers than maxEntriesPerScan). This will result in any ledger which were not in the scan, but which do exist, being gc'd.
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment29873>
From ZooKeeper? Surely from metastore
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment29874>
Again, the zk reference is incorrect.
- Ivan Kelly
On Nov. 13, 2012, 4:53 a.m., Jiannan Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8035/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 4:53 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> Implement a MetaStore based ledger manager for bookkeeper client.
>
>
> This addresses bug BOOKKEEPER-205.
> https://issues.apache.org/jira/browse/BOOKKEEPER-205
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java e6a3807
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 2692fde
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 16c0276
> bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java d528a18
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 2f39536
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
>
> Diff: https://reviews.apache.org/r/8035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiannan Wang
>
>
Re: Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger
manager for bookkeeper client.
Posted by Jiannan Wang <ji...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/
-----------------------------------------------------------
(Updated Dec. 6, 2012, 9:36 a.m.)
Review request for bookkeeper.
Changes
-------
Change follow Ivan and Fangmin's comments.
Test Total time that without MSLedgerManagerFactory: 15:17.931s
Test Total time that involve MSLedgerManagerFactory: 21:24.679s
Description
-------
Implement a MetaStore based ledger manager for bookkeeper client.
This addresses bug BOOKKEEPER-205.
https://issues.apache.org/jira/browse/BOOKKEEPER-205
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java 749fdb9
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 536403a
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 9d38dd0
bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java 35151df
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 12e4afc
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
Diff: https://reviews.apache.org/r/8035/diff/
Testing
-------
Thanks,
Jiannan Wang
Re: Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger
manager for bookkeeper client.
Posted by Jiannan Wang <ji...@yahoo-inc.com>.
> On Nov. 13, 2012, 11:21 a.m., Fangmin Lv wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 591
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line591>
> >
> > I'm afraid the algorithm can not contain all the ledger id ranges. For example:
> > Suppose bookie and metastore both contain L1, L2, ..., Ln ledgers initially, then during the gc interval L1, L2, L(n-1) were deleted from metastore, so only Ln will be scaned out, the range would be [n,n], then L1, L2, ..., L(n-1) may never deleted in the feature. I think the startLedgerId need to start from 0, also need to check the last range which from max ledger id in metastore to bookie max active ledgers.
1. startLedgerId is initial to 0 at the beginning of GC (line 528). For each sub gc round, it needn't to restart from 0. cursor.readEntries() always keeps the cursor moving.
2. Yeah, it's the same problem with line 573, after finish scanning all active ledger id in metastore, further processing is needed.
> On Nov. 13, 2012, 11:21 a.m., Fangmin Lv wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 582
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line582>
> >
> > I think this is not correct, need to change to
> >
> > if (!entries.hasNext())
good catch
> On Nov. 13, 2012, 11:21 a.m., Fangmin Lv wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java, line 573
> > <https://reviews.apache.org/r/8035/diff/1/?file=189045#file189045line573>
> >
> > It seems miss the case that all ledgers were deleted, and need to gc all bookie active ledgers.
good catch!
- Jiannan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/#review13389
-----------------------------------------------------------
On Nov. 13, 2012, 4:53 a.m., Jiannan Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8035/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 4:53 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> Implement a MetaStore based ledger manager for bookkeeper client.
>
>
> This addresses bug BOOKKEEPER-205.
> https://issues.apache.org/jira/browse/BOOKKEEPER-205
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java e6a3807
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 2692fde
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 16c0276
> bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java d528a18
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 2f39536
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
>
> Diff: https://reviews.apache.org/r/8035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiannan Wang
>
>
Re: Review Request: [BOOKKEEPER-205] Implement a MetaStore based ledger
manager for bookkeeper client.
Posted by Fangmin Lv <lv...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8035/#review13389
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment28696>
It seems miss the case that all ledgers were deleted, and need to gc all bookie active ledgers.
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment28697>
I think this is not correct, need to change to
if (!entries.hasNext())
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
<https://reviews.apache.org/r/8035/#comment28698>
I'm afraid the algorithm can not contain all the ledger id ranges. For example:
Suppose bookie and metastore both contain L1, L2, ..., Ln ledgers initially, then during the gc interval L1, L2, L(n-1) were deleted from metastore, so only Ln will be scaned out, the range would be [n,n], then L1, L2, ..., L(n-1) may never deleted in the feature. I think the startLedgerId need to start from 0, also need to check the last range which from max ledger id in metastore to bookie max active ledgers.
- Fangmin Lv
On Nov. 13, 2012, 4:53 a.m., Jiannan Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8035/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 4:53 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> Implement a MetaStore based ledger manager for bookkeeper client.
>
>
> This addresses bug BOOKKEEPER-205.
> https://issues.apache.org/jira/browse/BOOKKEEPER-205
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java e6a3807
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 2692fde
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 16c0276
> bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java d528a18
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java ead3494
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java 2f39536
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java 9630d46
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java dd3450c
>
> Diff: https://reviews.apache.org/r/8035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jiannan Wang
>
>