[
https://issues.apache.org/jira/browse/HBASE-4027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13081962#comment-13081962
]
[email protected] commented on HBASE-4027:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1214/#review1363
-----------------------------------------------------------
CHANGES.txt
<https://reviews.apache.org/r/1214/#comment3063>
your diff appears to revert this change. perhaps you need to rebase on
trunk before you take diff against it.
src/main/java/org/apache/hadoop/hbase/io/hfile/CacheStats.java
<https://reviews.apache.org/r/1214/#comment3064>
style:
/**
* ...
*/
public class CacheStates {
(comment formatting and space before '{')
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment3068>
hrm, is this constructor ever meant to be used? If the off-heap cache isn't
configured, then it should just instantiate LruBlockCache directly, no?
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment3069>
does it ever make sense to have offHeapSize < onHeapSize? Perhaps we should
have a Preconditions check here?
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment3065>
hyphenate 'on-heap' and 'off-heap' for clarity
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment3066>
missing space - " bytes ..."
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment3067>
same as above
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1214/#comment3070>
we should add in the heap size used by the accounting and hashmaps in the
off-heap cache as well.
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment3071>
vertically collapse this - one line per param
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment3072>
when you check up front here, you end up doing two lookups in backingmap.
Since this is just a safety check, you could instead check the return value of
put() below. Something like:
ByteBuffer storedBlock = ...allloc
... fill it in...
ByteBuffer alreadyCached = backingMap.put(blockName, storedBlock);
if (alreadyCached != null) {
// we didn't insert the new one, so free it and throw an exception
backingStore.free(storedBlock);
throw new RuntimeException("already cached xxxxx");
}
make sense?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment3073>
I think there's a bug here if you have multiple users hammering the same
contentBlock -- two people can get to "rewind()" at the same time. You probably
need synchronized(contentBlock) around these two lines. See if you can add a
unit test which puts just one block in the cache and starts several threads
which hammer it - I bet you eventually one of the blocks comes back returned as
all 0x0000
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment3074>
this.size() is in units of bytes, not blocks
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SingleSizeCache.java
<https://reviews.apache.org/r/1214/#comment3075>
maybe rename to getOccupiedSize?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment3076>
wrong Log class - should use org.apache.commons.logging.Log
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment3077>
remove extra whitespace
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment3078>
hm, we have 4 different terms for these: buffers, items, chunks, and
blocks. Can we have a terminology that's used consistently throughout?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment3079>
LOG.warn("Shutdown failed!", e); is probably what you want. Also improve
the text of this error message -- eg "Unable to deallocate direct memory during
shutdown".
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment3080>
getBlock*s*Remaining, right?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/Slab.java
<https://reviews.apache.org/r/1214/#comment3081>
incomplete comment here
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3083>
this needs to also make it a daemon thread, right? You could also look into
org.apache.hadoop.hbase.Chore
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3085>
this isn't calling addSlabByConf here, so its javadoc seems out of date?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3087>
spelling: proportions
Also make the confs more heirarchical - hbase.offheapcache.slab.sizes or
something is easier to parse. If we have any other configs throughout, they
should all share a prefix
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3088>
"configuration mismatch" is vague for users to understand. Better to say
something like "hbase.offheapslabproportions specifies proportions for 4 slab
classes, whereas hbase.offheapslabsizes specifies sizes for 5 slab classes." so
they can see specifically what the issue is.
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3094>
just to be thorough, check that they aren't negative?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3089>
> 0, not == 1 -- the contract of compareTo is just that it returns
positive, not that it returns exactly 1
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3091>
should include the name of the config here too - "Sum of all proportions
specified in hbase.blahblah is greater than 1."
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3095>
should we also check that they sum up to at least 0.99 or something?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3092>
wrap line
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3093>
you should .trim() the strings so whitespace is ignored
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3096>
is that true? I thought it threw an RTE
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3097>
collapse vertical space
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3098>
this is a little gross, since the APIs imply that you can cache anything,
but the implementation only supports HFileBlock.
In the HFile code review I'd suggested adding a new interface like
CacheableBlock that would expose any APIs you need. We should consider doing
that now?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3100>
javadoc to indicate where this is called from.
Since this is only used as the callback for the per-size caches, I think it
would be better to hide it by making it private, and not having this class
implement SlabItemEvictionWatcher. Then when you instantiate the subclasses,
pass them an anonymous class:
new SlabItemEvictionWatcher() {
public void onEviction(String key) {
internalBlockEvicted(key);
}
}
that way it doesn't show up as a public API to anyone else
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3102>
I think that the assignedCache's evict() counter is going to be double
incremented here... call stack like:
SingleSizeCache.ConcurrentMap.eviction
-> SingleSizeCache's eviction watcher
---> calls stats.evict()
---> removes from backingMap
---> calls SlabCache.onEviction()
-----> calls assignedCache.evict(key)
-------> calls stats.evict() again
-------> removes from map which is always a no-op
right?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3103>
may as well just iterate over sizer.values()
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3104>
again can iterate over values()
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3105>
move this function up in the file so it's with other functions. Also needs
@Override right?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3106>
also move this function up above the inner classes
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabCache.java
<https://reviews.apache.org/r/1214/#comment3099>
does this make a copy? it shouldn't need to at this point, right?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabItemEvictionWatcher.java
<https://reviews.apache.org/r/1214/#comment3108>
this can be package-private right?
src/main/java/org/apache/hadoop/hbase/io/hfile/slab/SlabItemEvictionWatcher.java
<https://reviews.apache.org/r/1214/#comment3107>
you don't need abstract inside an interface
src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
<https://reviews.apache.org/r/1214/#comment3109>
see note about consistency of conf params above
src/main/java/org/apache/hadoop/hbase/util/DirectMemoryUtils.java
<https://reviews.apache.org/r/1214/#comment3110>
Apache license
src/main/java/org/apache/hadoop/hbase/util/DirectMemoryUtils.java
<https://reviews.apache.org/r/1214/#comment3111>
this is unused?
src/main/java/org/apache/hadoop/hbase/util/DirectMemoryUtils.java
<https://reviews.apache.org/r/1214/#comment3112>
remove empty javadocs
src/main/java/org/apache/hadoop/hbase/util/FSMapRUtils.java
<https://reviews.apache.org/r/1214/#comment3113>
patch accidentally reverting another recent commit
src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3114>
add license
src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3115>
long line
src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3117>
using a queue here ensures that two threads never request the same block in
this test - that's one good test, but there should be another that just caches
a small number of blocks and pounds on get only
src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3116>
I think this amount of output is going to severaly limit the throughput at
which the threads can pound on the cache. Perhaps below:
long total = totalQueries.incrementAndGet();
if (total % 10000 == 0) {
LOG.debug("Ran " + total + " queries");
}
src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3118>
rather than changing the implementation of equals() in HFileBlock, I think
it's better to add a static method like blocksContainSameData() here in the
tests -- otherwise it might cause problems elsewhere where we were relying on
identity-equality for HFileBlock.equals
src/test/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3120>
this class looks like mostly dup code
src/test/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCacheTestUtils.java
<https://reviews.apache.org/r/1214/#comment3119>
hrm, this is identical to the other method?
- Todd
On 2011-08-09 20:39:55, 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-08-09 20:39:55)
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. CHANGES.txt e9c0478
bq. conf/hbase-env.sh 2d55d27
bq. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
aa09b7d
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java 2d4002c
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/HFileBlock.java 097dc50
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
88aa652
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java
886c31d
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
86652c0
bq. src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
94c8bb4
bq. src/main/java/org/apache/hadoop/hbase/util/DirectMemoryUtils.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/util/FSMapRUtils.java e70b0d4
bq.
src/test/java/org/apache/hadoop/hbase/io/hfile/HFileBlockCacheTestUtils.java
PRE-CREATION
bq.
src/test/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCacheTestUtils.java
PRE-CREATION
bq.
src/test/java/org/apache/hadoop/hbase/io/hfile/slab/TestSingleSizeCache.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. src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
4387170
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, 4027v7.diff, HBase-4027 (1).pdf,
> HBase-4027.pdf, HBase4027v8.diff, HBase4027v9.diff, hbase-4027-v10.5.diff,
> hbase-4027-v10.diff, hbase-4027v10.6.diff, 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
