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 2012/03/26 16:45:30 UTC
Review Request: BOOKKEEPER-193: Ledger is garbage collected by mistake.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4481/
-----------------------------------------------------------
Review request for bookkeeper.
Summary
-------
create a snapshot of bookie active ledgers, and then fetch the list of zookeeper metadata, then it is safe to do garbage collection.
This addresses bug BOOKKEEPER-193.
https://issues.apache.org/jira/browse/BOOKKEEPER-193
Diffs
-----
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 9d9bf22
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 100cdad
Diff: https://reviews.apache.org/r/4481/diff
Testing
-------
Thanks,
Sijie
Re: Review Request: BOOKKEEPER-193: Ledger is garbage collected by mistake.
Posted by Sijie Guo <gu...@gmail.com>.
> On 2012-03-28 08:10:21, fpj wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java, line 124
> > <https://reviews.apache.org/r/4481/diff/1/?file=95778#file95778line124>
> >
> > Can we perhaps use another latch here instead of time? Relying on time doesn't always work and in many cases it will induce unnecessary waiting time.
agreed. it could use another latch. will fix it in new patch.
> On 2012-03-28 08:10:21, fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java, line 37
> > <https://reviews.apache.org/r/4481/diff/1/?file=95777#file95777line37>
> >
> > Possibly not a big deal, but I wonder if it is really necessary to use this lock. In the way I read the code, I think it would work without it, but perhaps I'm missing something important.
the lock is used to avoid modifying two variables updates & updatesToMerge map, which is used to avoid inserting to updatesToMerge map during snapshot.
suppose two thread, one is doing insertion, the other one is done snapshot.
1) insertion thread: get the reference of updates map, tried to call #put.
2) snapshot thread: swap updates map to updatesToMerge map.
3) snapshot thread: iterates over the updatesToMerge map to merge them to snapshot map. (it may take times)
4) insertion thread: execute put. since updates map object has been assigned to updatesToMerge, so the put actually is applied in updatesToMerge, if the insertion position is large than the current position of iteration, it is OK. otherwise, the insertion will be lost.
the readwrite lock only blocks when modifying the updates & updatesToMerge references, I think it would not be expensive.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4481/#review6481
-----------------------------------------------------------
On 2012-03-26 14:45:30, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4481/
> -----------------------------------------------------------
>
> (Updated 2012-03-26 14:45:30)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> create a snapshot of bookie active ledgers, and then fetch the list of zookeeper metadata, then it is safe to do garbage collection.
>
>
> This addresses bug BOOKKEEPER-193.
> https://issues.apache.org/jira/browse/BOOKKEEPER-193
>
>
> Diffs
> -----
>
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 9d9bf22
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 100cdad
>
> Diff: https://reviews.apache.org/r/4481/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-193: Ledger is garbage collected by mistake.
Posted by fp...@apache.org.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4481/#review6481
-----------------------------------------------------------
It looks very good, Sijie. I have just a few points.
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java
<https://reviews.apache.org/r/4481/#comment14130>
I really like the idea of using a snapshot map!!
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java
<https://reviews.apache.org/r/4481/#comment14131>
Possibly not a big deal, but I wonder if it is really necessary to use this lock. In the way I read the code, I think it would work without it, but perhaps I'm missing something important.
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
<https://reviews.apache.org/r/4481/#comment14132>
Can we perhaps use another latch here instead of time? Relying on time doesn't always work and in many cases it will induce unnecessary waiting time.
- fpj
On 2012-03-26 14:45:30, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4481/
> -----------------------------------------------------------
>
> (Updated 2012-03-26 14:45:30)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> create a snapshot of bookie active ledgers, and then fetch the list of zookeeper metadata, then it is safe to do garbage collection.
>
>
> This addresses bug BOOKKEEPER-193.
> https://issues.apache.org/jira/browse/BOOKKEEPER-193
>
>
> Diffs
> -----
>
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 9d9bf22
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 100cdad
>
> Diff: https://reviews.apache.org/r/4481/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-193: Ledger is garbage collected by mistake.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4481/#review6511
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java
<https://reviews.apache.org/r/4481/#comment14165>
Casting indicates that something is wrong in your abstraction. Perhaps activeLedgers.snapshot() should return a SortedMap, and doGC() should take a Map. It doesn't do anything that requires the ConcurrentMap interface.
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java
<https://reviews.apache.org/r/4481/#comment14166>
why not just have the implementation of SnapshotMap use a ConcurrentSkipListMap?
- Ivan
On 2012-03-29 05:27:14, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4481/
> -----------------------------------------------------------
>
> (Updated 2012-03-29 05:27:14)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> create a snapshot of bookie active ledgers, and then fetch the list of zookeeper metadata, then it is safe to do garbage collection.
>
>
> This addresses bug BOOKKEEPER-193.
> https://issues.apache.org/jira/browse/BOOKKEEPER-193
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 100cdad
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 169c906
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/4481/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-193: Ledger is garbage collected by mistake.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4481/
-----------------------------------------------------------
(Updated 2012-03-29 05:27:14.643196)
Review request for bookkeeper.
Changes
-------
improve patch to use latch instead of time.
Summary
-------
create a snapshot of bookie active ledgers, and then fetch the list of zookeeper metadata, then it is safe to do garbage collection.
This addresses bug BOOKKEEPER-193.
https://issues.apache.org/jira/browse/BOOKKEEPER-193
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/SnapshotMap.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java 100cdad
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java 169c906
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/LedgerManagerTestCase.java PRE-CREATION
Diff: https://reviews.apache.org/r/4481/diff
Testing
-------
Thanks,
Sijie