> On 2011-11-24 18:58:01, Ivan Kelly wrote: > > Looking good. There's some cleanup and simplifications to do, but the patch > > is close now. I think it may take 1 or 2 more iterations, but this is a big > > patch, so that's pretty normal. A general comment is that scope creeped a > > little in the last patch with LedgerLayout and the dynamic loading changes. > > These are separate patches, and should be in separate JIRAs. Having the > > content of a patch focused on one thing makes it much easier to review, > > especially when the main part is as big as it is in this one. This is > > something I did too when I started sending patches to Apache, but I found > > quite quickly that the smaller the changes, the quicker things get review :)
I agreed. Let's focus on LedgerManager interface itself. I will remove dynamic loading changes and ledger layout util integrated with key/value storage (such as hbase). > On 2011-11-24 18:58:01, Ivan Kelly wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 133 > > <https://reviews.apache.org/r/2817/diff/2/?file=59756#file59756line133> > > > > There is only ever one instance of entrylogger, which has one instance > > of GarbageCollectorThread. Why is this needed? > > > > I think what you're intend may be here (correct me if im wrong) may be > > that you don't want doGcLedger() to be called more than once in parallel. > > doGcLedgers and doGcEntryLogs are async calls, so this synchronization > > method won't work for that case. Your guess is right. seems that gc code are now sync calls, so the flag can be removed. > On 2011-11-24 18:58:01, Ivan Kelly wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 250 > > <https://reviews.apache.org/r/2817/diff/2/?file=59756#file59756line250> > > > > While is correct here (when is also correct, but changes the meaning > > slightly). I try to fix the typo of 'colected' to 'collected', but I changed 'when' by mistake. > On 2011-11-24 18:58:01, Ivan Kelly wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java, > > line 70 > > <https://reviews.apache.org/r/2817/diff/2/?file=59757#file59757line70> > > > > activeLedgerManager should be set to alm before here. The check in the > > gcThread for activeLedgerManager isn't very nice. In fact it's not needed. > > If you look at the old code, the check is entryLogger.activeLedgers == > > null, which can never be the case as activeLedgers is assigned to a new > > HashMap on construction. Agree. > On 2011-11-24 18:58:01, Ivan Kelly wrote: > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java, > > line 1 > > <https://reviews.apache.org/r/2817/diff/2/?file=59768#file59768line1> > > > > This is barely used at the moment and shouldn't be in this patch. It is > > something we need to address in the future, but it's better to keep patches > > focused on one thing, which in this case is the ledger manager interface. Agreed. - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2817/#review3504 ----------------------------------------------------------- On 2011-11-22 10:39:27, Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2817/ > ----------------------------------------------------------- > > (Updated 2011-11-22 10:39:27) > > > Review request for bookkeeper. > > > Summary > ------- > > Create a interface LedgerManager to handle 'how to store ledger meta in > zookeeper, how bookie server manages active ledgers and garbage collect those > inactive/deleted ledgers'. Implement this interface using hash mechanism, > which hides detail from client/server code and makes code more clearer. > > > This addresses bug BOOKKEEPER-39. > http://issues.apache.org/jira/browse/BOOKKEEPER-39 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/conf/bk_server.conf > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/CallbackUtils.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/StringUtils.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ZkUtils.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java > 1204867 > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/LedgerLayoutTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalAsyncLedgerOpsTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalBookieFailureTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalBookieReadWriteTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalBookieRecoveryTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/hierarchical/HierarchicalLedgerDeleteTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/zookeeper/bookkeeper/trunk/doc/bookkeeperConfig.textile > 1204867 > > Diff: https://reviews.apache.org/r/2817/diff > > > Testing > ------- > > > Thanks, > > Sijie > >
