[
https://issues.apache.org/jira/browse/HBASE-4027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13070862#comment-13070862
]
jirapos...@reviews.apache.org commented on HBASE-4027:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1191/#review1182
-----------------------------------------------------------
could do with some tests for MetaSlab. also some multi-threaded tests - see
MultithreadedTestUtil, example usage in TestMemStoreLAB
pom.xml
<https://reviews.apache.org/r/1191/#comment2484>
did you determine that this ConcurrentLinkedHashMap was different than the
one in Guava? I thought it got incorporated into Guava, which we already depend
on.
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2485>
punctuation wise, I think it would be easier to read if you hyphenated
on-heap and off-heap. This applies to log messages below as well.
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2486>
No need to line-break here
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2487>
consider using StringUtils.humanReadableInt for these sizes.
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2488>
@Override
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2489>
when you're just overriding something from the superclass, no need for
javadoc unless it says something new and exciting. If you feel like you want to
put something there, you can use /** {@inheritDoc} */ to be explicit that
you're inheriting from the superclass.
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2490>
I think you should only put-back into the on-heap cache in the case that
the 'caching' parameter is true.
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2491>
hrm, the class javadoc says that the statistics should be cumulative, but
this seems to just forward
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2492>
TODOs
src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
<https://reviews.apache.org/r/1191/#comment2493>
is this code used? seems like dead copy-paste code to me.
src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
<https://reviews.apache.org/r/1191/#comment2497>
extraneous debugging left in
src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
<https://reviews.apache.org/r/1191/#comment2498>
I think this is usually called a "slab class" - I think that name would be
less confusing, since "Meta" is already used in HBase to refer to .META.
src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
<https://reviews.apache.org/r/1191/#comment2499>
unclear what the difference is between the two.
Is "slabs" the list of 2GB buffers, and "buffers" is the list of actual
items that will be allocated? I think the traditional names here are "slabs"
and "items". where each slab holds some number of allocatable items
Also, rather than // comments, use /** javadoc comments */ before the vars
src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
<https://reviews.apache.org/r/1191/#comment2500>
these vars probably better called maxBlocksPerSlab and maxSlabSize, since
they're upper bounds.
src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
<https://reviews.apache.org/r/1191/#comment2501>
I think this code would be a little easier to understand if you split it
into one loop for the full slabs, and an if statement for the partially full
one. Something like:
int numFullSlabs = numBlocks / maxBlocksPerSlab;
boolean hasPartialSlab = (numBlocks % maxBlocksPerSlab) > 0;
for (int i = 0; i < numFullSlabs; i++) {
alloc one of maxSlabSize;
addBuffersForSlab(slab);
}
if (hasPartialSlab) {
alloc the partial one
addBuffersForSlab(slab);
}
src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
<https://reviews.apache.org/r/1191/#comment2502>
should be a LOG.warn
src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
<https://reviews.apache.org/r/1191/#comment2503>
shouldn't this class have an alloc() and free() method?
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2511>
shouldn't this implement BlockCache?
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2504>
no need to line-break
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2505>
It seems dirty to reach back upwards to "master" here. Cyclical
dependencies are a code smell...
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2508>
I don't understand what purpose returnMap is serving here
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2506>
add '@Override', then you don't need to copy-paste javadoc
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2507>
this is where you would call backingStore.alloc() -- then buffers can be
made into a private member and keep encapsulation
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2510>
@Override, remove javadoc
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2509>
this is super ugly, plus bad performance - throwing an exception is very
expensive, since it has to construct a whole stack trace object, etc.
Instead, check backingMap.get(key) against null
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2512>
why does master need to know about this?
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2495>
numBlocks isn't the size, is it?
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2496>
missing space before "blocks"
src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
<https://reviews.apache.org/r/1191/#comment2494>
there are several identical copies of this floating around. Can they all
use the same class?
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2513>
overall note: I think we could move all of this to a new io.hfile.slab
package
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2520>
should this implement BlockCache?
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2515>
since this is only modified during initialization, we can use an
unsynchronized TreeMap which is probably more efficient
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2516>
rename to statThreadPeriodSecs to indicate time unit.
Though, since it's a constant, it probably should be STAT_THREAD_PERIOD.
This should be configurable but we can address in a follow-up
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2525>
you can set the thread factory to make daemon threads, then you don't need
setDaemon(true) below
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2514>
line breaks unnecessary
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2517>
this stuff will have to be configurable too. but this is fine for now
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2518>
should also note that it returns 0 for the case of an uncacheable object
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2519>
this function is never used -- you should make it private, and then call it
from cacheBlock below.
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2521>
again this is no good. check for null instead of catching an exception
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2522>
when would this be true? evicting something that doesn't have an entry in
backingStore?
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2523>
it seems like there's a loop in flow control here.
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2524>
move this nice logging output to something like: slabstats.logUsage()
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2526>
since you're using scheduleAtFixedRate, this should implement Runnable, not
extend Thread
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2527>
need to be public?
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2528>
should be ALL_CAPS
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2529>
ALSO_ALL_CAPS
src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
<https://reviews.apache.org/r/1191/#comment2530>
why boxed longs?
src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
<https://reviews.apache.org/r/1191/#comment2531>
for consistency, I think we should name these something like:
hfile.block.cache.offheap.size
hfile.block.cache.offheap.minblocksize
also, is min block size really a min? isn't it more like expectedblocksize?
src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java
<https://reviews.apache.org/r/1191/#comment2532>
update this javadoc
src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java
<https://reviews.apache.org/r/1191/#comment2533>
should use JUnit 4 style tests in new code - i.e not inherit TestCase.
src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java
<https://reviews.apache.org/r/1191/#comment2535>
nowhere in this test does it actually verify that the data is valid
src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java
<https://reviews.apache.org/r/1191/#comment2534>
some javadoc would be nice... it seems this code is copy-pasted from
TestLruBlockCache, but really doesn't make sense when applied to the
SingleSlabCache
src/test/java/org/apache/hadoop/hbase/io/hfile/TestSlabCache.java
<https://reviews.apache.org/r/1191/#comment2536>
see above.
If you can refactor some of this code into something like
BlockCacheTestUtils.testBasicCacheContract(BlockCache cache) that would be
good... copy paste evil
- Todd
On 2011-07-25 22:55:56, Todd Lipcon wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1191/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-07-25 22:55:56)
bq.
bq.
bq. Review request for hbase and Todd Lipcon.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Uploading slabcachepatchv4 to review for Li Pi.
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. pom.xml 729dc37
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java 5963552
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
b600020
bq. src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/io/hfile/TestSlabCache.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/1191/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Todd
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: slabcachepatch.diff, slabcachepatchv2.diff,
> slabcachepatchv3.1.diff, slabcachepatchv3.2.diff, slabcachepatchv3.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
