> On Dec. 3, 2012, 4:08 p.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/SnapshotMap.java, > > line 31 > > <https://reviews.apache.org/r/8141/diff/6/?file=231852#file231852line31> > > > > You define this as a map, but use it as a Set. Why not just define a > > set? If you're going to use a map for some future patch, it would be > > worthwhile to create a SnapshotSet class which wraps this. Otherwise, just > > make this a SnapshotSet class. > > Fangmin Lv wrote: > This is the map used in previously ActiveLedgerManager, I think the main > reason why we use SnapshotMap but not a set is that there is no > ConcurrentHashSet. > > Ivan Kelly wrote: > Ah, I had forgotten about that. I think it's ok for the moment, though it > would be nice to wrap this with a SnapshotSet class in the future. (HashSet > is a wrapper around HashMap anyhow).
Yes, we can easily get a ConcurrentHashSet with Collections.newSetFromMap(new ConcurrentHashMap<Object,Boolean>()), but I doubt whether we need to do like this. > On Dec. 3, 2012, 4:08 p.m., Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java, > > line 520 > > <https://reviews.apache.org/r/8141/diff/6/?file=231847#file231847line520> > > > > Throwing Exception is bad. It requires everything up the stack to catch > > Exception. It's better to use your own Exception, or just throw up anything > > you get in the method. > > Fangmin Lv wrote: > Actrually different LedgerManagers' hasNext and next throws different > exceptions, so I throw the general Exception in the LedgerRangeIterator > interface, if I use the exception throw from the method then what should I > throw in the interface? > > Ivan Kelly wrote: > The problem with Exception is that its a base class for RuntimeException, > which means that anything using this class will end up catching > RuntimeExceptions also, which they shouldn't do. I think the best thing to do > here is to wrap with IOException, as that seems to be what we're using in > metadata where exceptions are needed. Your concern is reasonable , will wrap the Exception - Fangmin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8141/#review13973 ----------------------------------------------------------- On Dec. 5, 2012, 10:27 a.m., Fangmin Lv wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8141/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2012, 10:27 a.m.) > > > Review request for bookkeeper. > > > Description > ------- > > Main changes: > > 1. Refactor Garbage Collector Interface > 2. Change LedgerManager#deleteLedger to versioned delete > 3. Remove ActiveLedgerManager interface > 4. Move some common functions to StringUtils and ZkUtils > > > This addresses bug BOOKKEEPER-463. > https://issues.apache.org/jira/browse/BOOKKEEPER-463 > > > Diffs > ----- > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > 929be51 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollector.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java > cecb74a > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java > c3f5149 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCacheImpl.java > c8d2b21 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerDeleteOp.java > eae1f37 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java > 9dcb1b9 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ActiveLedgerManager.java > 542b498 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java > e284776 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManagerFactory.java > 329e0a7 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java > 3499a05 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java > c86b884 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java > 30e2b83 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java > a7fc247 > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java > c222f05 > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/SnapshotMap.java > PRE-CREATION > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/StringUtils.java > 575e480 > bookkeeper-server/src/main/java/org/apache/bookkeeper/util/ZkUtils.java > 4073450 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerCacheTest.java > a24b1e2 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java > 7ecf937 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java > cd0b91f > > Diff: https://reviews.apache.org/r/8141/diff/ > > > Testing > ------- > > > Thanks, > > Fangmin Lv > >
