[
https://issues.apache.org/jira/browse/HBASE-4027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13072940#comment-13072940
]
jirapos...@reviews.apache.org commented on HBASE-4027:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1214/#review1211
-----------------------------------------------------------
conf/hbase-env.sh
<https://reviews.apache.org/r/1214/#comment2675>
Is MaxDirectMemorySize determinable on the running jvm? Could we make the
offheapcachesize config as a percentage of the direct memory size like we have
for memstore/blockcache today? (default of 0.95 or something would make it so
it never really has to be set for most cases... and i'm not sure what exactly
"a bit above the off heap cache size" is)
src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment2676>
2011
src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment2677>
whitespace
src/main/java/org/apache/hadoop/hbase/io/hfile/CacheStats.java
<https://reviews.apache.org/r/1214/#comment2678>
license
src/main/java/org/apache/hadoop/hbase/io/hfile/CacheStats.java
<https://reviews.apache.org/r/1214/#comment2679>
class comment
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment2680>
whitespace here and throughout this file
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment2681>
Would it make sense to have DoubleBlockCache be more generic? Does it need
to be fixed with these two types or could it take two BlockCache's and they are
executed in the order they are given in (just need to be clear in doc).
If this was generic, it could be reused for various multi-level caches
(like an underlying cache with compressed blocks and one above it with
uncompressed blocks)
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment2682>
longer than 80 chars
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment2683>
This seems like a behavior that we may not always want.
If we made this class generic, could we have some kind of policy we
initiate it with? (like default cache in level one, if accessed in level one,
cache in level two, etc?)
we're going to always be double-storing anything so that the offHeap true
capacity is (totalOffHeap - totalOnHeap). in some cases, we might want to
cache on heap first and then if evicted we cache off heap, or maybe we want it
to work more like the existing LRU (first read goes into off heap, second read
upgrades it to the on heap cache and removes from the off heap)
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment2684>
this is going to make for some weird stats? seems like we may need to
actually expose the stats of each underlying cache rather than both? (or both
and separate). it's going to be difficult to understand what's happening when
the hit and eviction stats cover both.
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment2685>
huh?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment2686>
line > 80 chars
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment2687>
getTotalNumBlocks() and getRemainingNumBlocks() or something? i find the
method names a little unclear (or just add some javadoc)
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment2688>
javadoc on these
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2689>
I'm not totally clear on why the SlabCache contains a bunch of
SingleSizeCaches. Why do you need to layer BlockCaches on top of BlockCaches?
You'll have one slab per size rather than one cache per size? Can you not pass
the right evictor callback in so it goes back to the right slab?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2690>
Why these ratios? At the least, this should all be configurable (even if
just in code and undocumented).
Do we need to always pre-allocate everything and determine the block/slab
sizes and all that? The design seems inflexible because it's all determine
during construction rather than being adaptive.
I'm okay with the first iteration not being awesome and auto-tuning but
this layered cache design seems to make it hard to change anything once it's
instantiated.
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2691>
seems like an odd behavior. this method is kinda non-deterministic from
the caller POV, we have no idea whether this passed or failed
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2692>
is this really a cache miss? this cache will never take this block.
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2693>
but here we need to check for null and increment cache miss rather than hit
if it is null
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2694>
this non-javadoc comment is not necessary, just the @Override has been
normal format in hbase
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment2762>
should this really be a bunch of INFO logs? Could we have these exported
as metrics instead?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabItemEvictionWatcher.java
<https://reviews.apache.org/r/1214/#comment2763>
license
src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
<https://reviews.apache.org/r/1214/#comment2764>
formatting looks a little strange here (missing spaces and such)
src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
<https://reviews.apache.org/r/1214/#comment2765>
line > 80 chars
src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSingleSlabCache.java
<https://reviews.apache.org/r/1214/#comment2766>
2011
src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSingleSlabCache.java
<https://reviews.apache.org/r/1214/#comment2767>
This test looks more specific than testing "the concurrent LruBlockCache"
src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSingleSlabCache.java
<https://reviews.apache.org/r/1214/#comment2768>
you are missing a space here and throughout this file and some others...
our format is to always have a space before a {
src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSlab.java
<https://reviews.apache.org/r/1214/#comment2769>
license
- Jonathan
On 2011-07-28 23:02:50, Li Pi wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1214/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-07-28 23:02:50)
bq.
bq.
bq. Review request for hbase, Todd Lipcon, Ted Yu, Michael Stack, Jonathan
Gray, and Li Pi.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Review request - I apparently can't edit tlipcon's earlier posting of my
diff, so creating a new one.
bq.
bq.
bq. This addresses bug HBase-4027.
bq. https://issues.apache.org/jira/browse/HBase-4027
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. conf/hbase-env.sh 2d55d27
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java 509121d
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheTestUtils.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/CacheStats.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
ecab7ca
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java
150f54f
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabItemEvictionWatcher.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
1d5e3fa
bq. src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
b600020
bq.
src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSingleSlabCache.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSlab.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSlabCache.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/1214/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Ran benchmarks against it in HBase standalone mode. Wrote test cases for
all classes, multithreaded test cases exist for the cache.
bq.
bq.
bq. Thanks,
bq.
bq. Li
bq.
bq.
> Enable direct byte buffers LruBlockCache
> ----------------------------------------
>
> Key: HBASE-4027
> URL: https://issues.apache.org/jira/browse/HBASE-4027
> Project: HBase
> Issue Type: Improvement
> Reporter: Jason Rutherglen
> Assignee: Li Pi
> Priority: Minor
> Attachments: 4027-v5.diff, HBase-4027.pdf, hbase-4027v6.diff,
> slabcachepatch.diff, slabcachepatchv2.diff, slabcachepatchv3.1.diff,
> slabcachepatchv3.2.diff, slabcachepatchv3.diff, slabcachepatchv4.5.diff,
> slabcachepatchv4.diff
>
>
> Java offers the creation of direct byte buffers which are allocated outside
> of the heap.
> They need to be manually free'd, which can be accomplished using an
> documented {{clean}} method.
> The feature will be optional. After implementing, we can benchmark for
> differences in speed and garbage collection observances.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
