----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4603/#review6616 -----------------------------------------------------------
It looks pretty good for me, I just have a few small comments. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java <https://reviews.apache.org/r/4603/#comment14294> For clarity, can we rename it to MetaStoreException or MetadataStoreException? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java <https://reviews.apache.org/r/4603/#comment14397> MetaStoreException bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java <https://reviews.apache.org/r/4603/#comment14401> It doesn't look like we need to make this call public. If it is for testing, we could simply use inheritance, no? bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java <https://reviews.apache.org/r/4603/#comment14404> 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? bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ActiveLedgerManager.java <https://reviews.apache.org/r/4603/#comment14417> You may want to say bookie instead of server side. bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ActiveLedgerManager.java <https://reviews.apache.org/r/4603/#comment14416> 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. bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java <https://reviews.apache.org/r/4603/#comment14405> The comment here should not refer to zk, right? - fpj 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 > >
