You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Matteo Merli <mm...@yahoo-inc.com> on 2015/04/11 02:06:38 UTC
Review Request 33098: Release LedgerDescriptor and master-key objects
when not used anymore
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33098/
-----------------------------------------------------------
Review request for bookkeeper.
Bugs: BOOKKEEPER-852
https://issues.apache.org/jira/browse/BOOKKEEPER-852
Repository: bookkeeper-git
Description
-------
Maps with ledger descriptors and master-keys are not cleaned after a ledger gets deleted.
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java 45be763
Diff: https://reviews.apache.org/r/33098/diff/
Testing
-------
Thanks,
Matteo Merli
Re: Review Request 33098: Release LedgerDescriptor and master-key
objects when not used anymore
Posted by Sijie Guo <gu...@gmail.com>.
> On April 14, 2015, 7:08 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java, line 137
> > <https://reviews.apache.org/r/33098/diff/1/?file=924174#file924174line137>
> >
> > this should be expireAfterAccess, right?
> >
> > maybe make the expiration configurable?
>
> Matteo Merli wrote:
> The problem with expireAfterAccess is that it make every access to the cache to update the cache. Since we're checking the cache for each entry written/read, that could be 10s of thousand times per sec. The impact of using expireAfterAccess is clearly visible (in terms of CPU usage and mem allocations) in the profiler.
>
> We could make it configurable, though I'm not clear on how to define this configurable time in user perspective. Ideally, we shouldn't need to use a "cache", we should clean the map when a ledger is deleted, but the garbage collector is only notifying the LedgerStorage of the deletions.
>
> The simple point is that we just want to avoid leaking memory when ledger are created/deleted over the time.
I'd suggest hold off this change. Let's figure out if there is a way to do it in a clear way to clean the map when a ledger is either deleted or evicted, rather than cache.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33098/#review79994
-----------------------------------------------------------
On April 11, 2015, 12:06 a.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33098/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 12:06 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-852
> https://issues.apache.org/jira/browse/BOOKKEEPER-852
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Maps with ledger descriptors and master-keys are not cleaned after a ledger gets deleted.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java 45be763
>
> Diff: https://reviews.apache.org/r/33098/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33098: Release LedgerDescriptor and master-key
objects when not used anymore
Posted by Matteo Merli <mm...@yahoo-inc.com>.
> On April 14, 2015, 7:08 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java, line 137
> > <https://reviews.apache.org/r/33098/diff/1/?file=924174#file924174line137>
> >
> > this should be expireAfterAccess, right?
> >
> > maybe make the expiration configurable?
The problem with expireAfterAccess is that it make every access to the cache to update the cache. Since we're checking the cache for each entry written/read, that could be 10s of thousand times per sec. The impact of using expireAfterAccess is clearly visible (in terms of CPU usage and mem allocations) in the profiler.
We could make it configurable, though I'm not clear on how to define this configurable time in user perspective. Ideally, we shouldn't need to use a "cache", we should clean the map when a ledger is deleted, but the garbage collector is only notifying the LedgerStorage of the deletions.
The simple point is that we just want to avoid leaking memory when ledger are created/deleted over the time.
> On April 14, 2015, 7:08 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java, line 1097
> > <https://reviews.apache.org/r/33098/diff/1/?file=924174#file924174line1097>
> >
> > it would be better to check if e.getCause instanceof IOException.
Ok, will change that.
> On April 14, 2015, 7:08 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java, line 41
> > <https://reviews.apache.org/r/33098/diff/1/?file=924175#file924175line41>
> >
> > expireAfterAccess? configurable time?
Same as above comment
- Matteo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33098/#review79994
-----------------------------------------------------------
On April 11, 2015, 12:06 a.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33098/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 12:06 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-852
> https://issues.apache.org/jira/browse/BOOKKEEPER-852
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Maps with ledger descriptors and master-keys are not cleaned after a ledger gets deleted.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java 45be763
>
> Diff: https://reviews.apache.org/r/33098/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33098: Release LedgerDescriptor and master-key
objects when not used anymore
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33098/#review79994
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/33098/#comment129724>
this should be expireAfterAccess, right?
maybe make the expiration configurable?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/33098/#comment129725>
it would be better to check if e.getCause instanceof IOException.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java
<https://reviews.apache.org/r/33098/#comment129726>
expireAfterAccess? configurable time?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java
<https://reviews.apache.org/r/33098/#comment129727>
expireAfterAccess? configurable time?
- Sijie Guo
On April 11, 2015, 12:06 a.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33098/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 12:06 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-852
> https://issues.apache.org/jira/browse/BOOKKEEPER-852
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Maps with ledger descriptors and master-keys are not cleaned after a ledger gets deleted.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java 45be763
>
> Diff: https://reviews.apache.org/r/33098/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>