You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Ivan Kelly <iv...@apache.org> on 2012/12/03 17:08:00 UTC
Re: Review Request: Refactor garbage collection code for ease to plugin
different GC algorithm.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8141/#review13973
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
<https://reviews.apache.org/r/8141/#comment29892>
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.
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/SnapshotMap.java
<https://reviews.apache.org/r/8141/#comment29893>
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.
- Ivan Kelly
On Nov. 30, 2012, 7 a.m., Fangmin Lv wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8141/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 7 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
>
>
Re: Review Request: Refactor garbage collection code for ease to plugin
different GC algorithm.
Posted by Ivan Kelly <iv...@apache.org>.
> 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.
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).
> 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?
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.
- Ivan
-----------------------------------------------------------
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
>
>
Re: Review Request: Refactor garbage collection code for ease to plugin
different GC algorithm.
Posted by Fangmin Lv <lv...@gmail.com>.
> 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.
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?
> 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.
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.
- Fangmin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8141/#review13973
-----------------------------------------------------------
On Nov. 30, 2012, 7 a.m., Fangmin Lv wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8141/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 7 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
>
>
Re: Review Request: Refactor garbage collection code for ease to plugin
different GC algorithm.
Posted by Fangmin Lv <lv...@gmail.com>.
> 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
>
>