> On 2012-04-03 21:17:04, fpj wrote: > > It looks pretty good for me, I just have a few small comments.
Thanks Flavio for reviewing it. I would like to bring the patch to latest trunk and update it with your comments. > On 2012-04-03 21:17:04, fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java, > > line 178 > > <https://reviews.apache.org/r/4603/diff/1/?file=97866#file97866line178> > > > > It doesn't look like we need to make this call public. If it is for > > testing, we could simply use inheritance, no? we use parseConfig in ledgermanager not for testing, which is another package. > On 2012-04-03 21:17:04, fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java, > > line 146 > > <https://reviews.apache.org/r/4603/diff/1/?file=97868#file97868line146> > > > > I'm wondering if it is ok to have a reference specific to zk here. Is > > the idea here that for every new backend we add, we include required system > > properties here? zkLedgersRootPath not only for zookeeper-based ledger manager to store ledger metadata. under zkLedgersRootPath there is a 'layout' znode, which describes the metadata layout and which ledger manager it used for current bookkeeper cluster. > On 2012-04-03 21:17:04, fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ActiveLedgerManager.java, > > line 30 > > <https://reviews.apache.org/r/4603/diff/1/?file=97870#file97870line30> > > > > I wonder if ActiveLedgerManager is a good name for this class. It > > doesn't indicate that it is related to the bookie side, and it might be > > good to refer to it. But, I don't feel too strongly about it, since the > > comment before the class says that it is for bookies. I don't have a good name for it also, just used its original name before we abstracted ledger manager interface. - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4603/#review6616 ----------------------------------------------------------- On 2012-04-01 15:56:33, Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4603/ > ----------------------------------------------------------- > > (Updated 2012-04-01 15:56:33) > > > Review request for bookkeeper. > > > Summary > ------- > > we need to improve ledger manager interface to remove zookeeper dependency on > metadata operations, so it is easy for us to implement a MetaStore based > ledger manager. > > > This addresses bug BOOKKEEPER-203. > https://issues.apache.org/jira/browse/BOOKKEEPER-203 > > > Diffs > ----- > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > ee38862 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java > 2eed192 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCacheImpl.java > 17c70f1 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java > 911c660 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java > 490c130 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java > b9cd624 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java > f0a165f > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java > 5b10a5b > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java > e576792 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java > 2dcf0e4 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java > 56186ab > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java > e1bd1ff > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java > 8fd7df8 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ActiveLedgerManager.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java > 1300974 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManagerFactory.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java > b573181 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java > 3a3a81e > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java > 7d9eb96 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java > b62ad3a > > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ReflectionUtils.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerCacheTest.java > 52cf514 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java > 6135250 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestFencing.java > c5350ad > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java > 12c1b5a > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/HierarchicalAsyncLedgerOpsTest.java > 24b3c12 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/HierarchicalBookieFailureTest.java > 70a4ea9 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/HierarchicalBookieReadWriteTest.java > 7d97fd4 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/HierarchicalBookieRecoveryTest.java > d99156e > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/HierarchicalLedgerDeleteTest.java > dbcd65a > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerLayoutTest.java > 4784854 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java > b8a541b > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestLedgerManager.java > e07c756 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/AsyncLedgerOpsTest.java > d7f153a > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java > c172224 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieFailureTest.java > e647d68 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieReadWriteTest.java > 3a3c8af > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/LedgerDeleteTest.java > 2d3593e > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerBaseTestCase.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/4603/diff > > > Testing > ------- > > > Thanks, > > Sijie > >
