You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Sijie Guo <gu...@gmail.com> on 2015/08/12 04:21:14 UTC
Review Request 37381: BOOKKEEPER-438: Move ledger id generation out of
LedgerManager
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37381/
-----------------------------------------------------------
Review request for bookkeeper, fpj and Sijie Guo.
Bugs: BOOKKEEPER-438
https://issues.apache.org/jira/browse/BOOKKEEPER-438
Repository: bookkeeper-git
Description
-------
Move id generation out of LedgerManager to ensure different ledger manager implementation shared same ledger id space in ZooKeeper, which is necessary for migration between different ledger managers.
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java c5be32d
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java f74639b
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java 9cda8ca
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 3ec2b5a
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java f3f680d
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java a7fbcf5
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 2bc4258
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 7f2df73
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java b843e99
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerIdGenerator.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java 7229028
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java 7c3cf5c
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java 2510b89
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerIdGenerator.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 101fdac
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestWatchEnsembleChange.java eb833a3
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java b95d2db
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestLedgerIdGenerator.java PRE-CREATION
Diff: https://reviews.apache.org/r/37381/diff/
Testing
-------
Thanks,
Sijie Guo
Re: Review Request 37381: BOOKKEEPER-438: Move ledger id generation
out of LedgerManager
Posted by Sijie Guo <gu...@gmail.com>.
> On Aug. 12, 2015, 10:07 a.m., Ivan Kelly wrote:
> > The code looks good. However, I don't think backward compatability is handled. What happens if I have been generating ledger ids based on ephemeral sequential and then move to the new method. I'll start from scratch again. It should fail on creating the ledger znode, but what if the ledger has just been deleted and all it's entries are still on bookies? Perhaps each ledgermanager type should provide it's a default ledger id generator, which the user can then override.
Moved the LedgerIdGenerator into LedgerManagerFactory.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37381/#review95071
-----------------------------------------------------------
On Sept. 7, 2015, 10:38 p.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37381/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2015, 10:38 p.m.)
>
>
> Review request for bookkeeper, fpj and Sijie Guo.
>
>
> Bugs: BOOKKEEPER-438
> https://issues.apache.org/jira/browse/BOOKKEEPER-438
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Move id generation out of LedgerManager to ensure different ledger manager implementation shared same ledger id space in ZooKeeper, which is necessary for migration between different ledger managers.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java c5be32d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java f74639b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java 9cda8ca
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 3ec2b5a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java f3f680d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java a7fbcf5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 2bc4258
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManagerFactory.java db16d26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 7f2df73
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java b843e99
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerIdGenerator.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java 7229028
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java 7c3cf5c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java 2510b89
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerIdGenerator.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 101fdac
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestWatchEnsembleChange.java eb833a3
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java b95d2db
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestZkLedgerIdGenerator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37381/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie Guo
>
>
Re: Review Request 37381: BOOKKEEPER-438: Move ledger id generation
out of LedgerManager
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37381/#review95071
-----------------------------------------------------------
The code looks good. However, I don't think backward compatability is handled. What happens if I have been generating ledger ids based on ephemeral sequential and then move to the new method. I'll start from scratch again. It should fail on creating the ledger znode, but what if the ledger has just been deleted and all it's entries are still on bookies? Perhaps each ledgermanager type should provide it's a default ledger id generator, which the user can then override.
- Ivan Kelly
On Aug. 12, 2015, 2:21 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37381/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 2:21 a.m.)
>
>
> Review request for bookkeeper, fpj and Sijie Guo.
>
>
> Bugs: BOOKKEEPER-438
> https://issues.apache.org/jira/browse/BOOKKEEPER-438
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Move id generation out of LedgerManager to ensure different ledger manager implementation shared same ledger id space in ZooKeeper, which is necessary for migration between different ledger managers.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java c5be32d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java f74639b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java 9cda8ca
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 3ec2b5a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java f3f680d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java a7fbcf5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 2bc4258
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 7f2df73
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java b843e99
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerIdGenerator.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java 7229028
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java 7c3cf5c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java 2510b89
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerIdGenerator.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 101fdac
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestWatchEnsembleChange.java eb833a3
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java b95d2db
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestLedgerIdGenerator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37381/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie Guo
>
>
Re: Review Request 37381: BOOKKEEPER-438: Move ledger id generation
out of LedgerManager
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37381/#review98057
-----------------------------------------------------------
Ship it!
lgtm +1
- Ivan Kelly
On Sept. 7, 2015, 10:38 p.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37381/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2015, 10:38 p.m.)
>
>
> Review request for bookkeeper, fpj and Sijie Guo.
>
>
> Bugs: BOOKKEEPER-438
> https://issues.apache.org/jira/browse/BOOKKEEPER-438
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Move id generation out of LedgerManager to ensure different ledger manager implementation shared same ledger id space in ZooKeeper, which is necessary for migration between different ledger managers.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java c5be32d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java f74639b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java 9cda8ca
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 3ec2b5a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java f3f680d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java a7fbcf5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 2bc4258
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManagerFactory.java db16d26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 7f2df73
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java b843e99
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerIdGenerator.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java 7229028
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java 7c3cf5c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java 2510b89
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerIdGenerator.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 101fdac
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestWatchEnsembleChange.java eb833a3
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java b95d2db
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestZkLedgerIdGenerator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/37381/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie Guo
>
>
Re: Review Request 37381: BOOKKEEPER-438: Move ledger id generation
out of LedgerManager
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37381/
-----------------------------------------------------------
(Updated Sept. 7, 2015, 10:38 p.m.)
Review request for bookkeeper, fpj and Sijie Guo.
Changes
-------
addressed comments. move LedgerIdGenerator into LedgerManagerFactory.
Bugs: BOOKKEEPER-438
https://issues.apache.org/jira/browse/BOOKKEEPER-438
Repository: bookkeeper-git
Description
-------
Move id generation out of LedgerManager to ensure different ledger manager implementation shared same ledger id space in ZooKeeper, which is necessary for migration between different ledger managers.
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java c5be32d
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java f74639b
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java 9cda8ca
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java 3ec2b5a
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java f3f680d
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java a7fbcf5
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 2bc4258
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManagerFactory.java db16d26
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 7f2df73
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManagerFactory.java b843e99
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerIdGenerator.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java 7229028
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java 7c3cf5c
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java 2510b89
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerIdGenerator.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 101fdac
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestWatchEnsembleChange.java eb833a3
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java b95d2db
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/TestZkLedgerIdGenerator.java PRE-CREATION
Diff: https://reviews.apache.org/r/37381/diff/
Testing
-------
Thanks,
Sijie Guo