[
https://issues.apache.org/jira/browse/HBASE-4027?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13070965#comment-13070965
]
[email protected] commented on HBASE-4027:
------------------------------------------------------
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > pom.xml, line 712
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26914#file26914line712>
bq. >
bq. > 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.
Changed to MapMaker. Guava has deprecated some of the functions I'm using in
trunk, but hasn't provided a release with a replacement. But we'll deal with
that when the time comes.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
line 33
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line33>
bq. >
bq. > 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.
Done
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
lines 52-55
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line52>
bq. >
bq. > No need to line-break here
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
lines 64-65
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line64>
bq. >
bq. > consider using StringUtils.humanReadableInt for these sizes.
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
line 72
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line72>
bq. >
bq. > @Override
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
lines 77-85
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line77>
bq. >
bq. > 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.
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
line 102
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line102>
bq. >
bq. > I think you should only put-back into the on-heap cache in the case
that the 'caching' parameter is true.
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
line 128
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line128>
bq. >
bq. > hrm, the class javadoc says that the statistics should be
cumulative, but this seems to just forward
Fixed. Also refactored CacheStats into a separate class.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
lines 148-166
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line148>
bq. >
bq. > TODOs
Implemented these.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/DoubleBlockCache.java,
line 168
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26915#file26915line168>
bq. >
bq. > is this code used? seems like dead copy-paste code to me.
It is now.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java, line 1125
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26916#file26916line1125>
bq. >
bq. > extraneous debugging left in
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java, line 31
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26917#file26917line31>
bq. >
bq. > 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.
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java, lines 32-34
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26917#file26917line32>
bq. >
bq. > unclear what the difference is between the two.
bq. >
bq. > 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
bq. >
bq. > Also, rather than // comments, use /** javadoc comments */ before
the vars
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java, line 46
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26917#file26917line46>
bq. >
bq. > these vars probably better called maxBlocksPerSlab and maxSlabSize,
since they're upper bounds.
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java, lines 49-55
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26917#file26917line49>
bq. >
bq. > 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:
bq. >
bq. > int numFullSlabs = numBlocks / maxBlocksPerSlab;
bq. > boolean hasPartialSlab = (numBlocks % maxBlocksPerSlab) > 0;
bq. >
bq. > for (int i = 0; i < numFullSlabs; i++) {
bq. > alloc one of maxSlabSize;
bq. > addBuffersForSlab(slab);
bq. > }
bq. >
bq. > if (hasPartialSlab) {
bq. > alloc the partial one
bq. > addBuffersForSlab(slab);
bq. > }
bq. >
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java, lines 77-78
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26917#file26917line77>
bq. >
bq. > should be a LOG.warn
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 43
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line43>
bq. >
bq. > shouldn't this implement BlockCache?
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
lines 58-65
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line58>
bq. >
bq. > no need to line-break
Eclipse automatically does this. Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 67
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line67>
bq. >
bq. > It seems dirty to reach back upwards to "master" here. Cyclical
dependencies are a code smell...
If the SingleSizeSlab decides to evict, the master should evict as well.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 70
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line70>
bq. >
bq. > I don't understand what purpose returnMap is serving here
ReturnMap is pretty much how free() is implemented. I probably should move this
to MetaSlab - now Slab
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/MetaSlab.java, line 94
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26917#file26917line94>
bq. >
bq. > shouldn't this class have an alloc() and free() method?
Moving alloc() and free() functions from SingleSizeCache to MetaSlab. Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 106
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line106>
bq. >
bq. > this is where you would call backingStore.alloc() -- then buffers
can be made into a private member and keep encapsulation
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 123
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line123>
bq. >
bq. > @Override, remove javadoc
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
lines 96-99
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line96>
bq. >
bq. > add '@Override', then you don't need to copy-paste javadoc
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
lines 131-133
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line131>
bq. >
bq. > this is super ugly, plus bad performance - throwing an exception is
very expensive, since it has to construct a whole stack trace object, etc.
bq. >
bq. > Instead, check backingMap.get(key) against null
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 155
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line155>
bq. >
bq. > why does master need to know about this?
Master keeps its own records for which SingleSizeCache the key is assigned to.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 164
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line164>
bq. >
bq. > numBlocks isn't the size, is it?
Good catch. Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 166
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line166>
bq. >
bq. > missing space before "blocks"
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SingleSizeCache.java,
line 207
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26918#file26918line207>
bq. >
bq. > there are several identical copies of this floating around. Can they
all use the same class?
Refactored out and fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 21
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line21>
bq. >
bq. > overall note: I think we could move all of this to a new
io.hfile.slab package
Will do next revision.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 44
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line44>
bq. >
bq. > should this implement BlockCache?
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 47
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line47>
bq. >
bq. > since this is only modified during initialization, we can use an
unsynchronized TreeMap which is probably more efficient
Fixed. Although, in the future, we may want to modify it on the fly, which is
why it was a ConcurrentSkipListMap.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 49
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line49>
bq. >
bq. > rename to statThreadPeriodSecs to indicate time unit.
bq. >
bq. > 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
fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 53
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line53>
bq. >
bq. > you can set the thread factory to make daemon threads, then you
don't need setDaemon(true) below
changed to runnable.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, lines
65-68
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line65>
bq. >
bq. > line breaks unnecessary
eclipse autoformatting :(. fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, lines
80-85
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line80>
bq. >
bq. > this stuff will have to be configurable too. but this is fine for now
next revision!
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 95
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line95>
bq. >
bq. > should also note that it returns 0 for the case of an uncacheable
object
changed so it returns the entry instead of an int. returns null if unreachable.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 98
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line98>
bq. >
bq. > this function is never used -- you should make it private, and then
call it from cacheBlock below.
fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, lines
145-148
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line145>
bq. >
bq. > again this is no good. check for null instead of catching an
exception
fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 174
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line174>
bq. >
bq. > when would this be true? evicting something that doesn't have an
entry in backingStore?
It'll be true because theres a callback from SlabCache.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 177
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line177>
bq. >
bq. > it seems like there's a loop in flow control here.
Nope, SingleSlabCache should return false, and not loop.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 214
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line214>
bq. >
bq. > move this nice logging output to something like: slabstats.logUsage()
Done.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 229
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line229>
bq. >
bq. > since you're using scheduleAtFixedRate, this should implement
Runnable, not extend Thread
Done
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 250
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line250>
bq. >
bq. > need to be public?
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 253
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line253>
bq. >
bq. > should be ALL_CAPS
Done
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 254
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line254>
bq. >
bq. > ALSO_ALL_CAPS
Done.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/io/hfile/SlabCache.java, line 268
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26919#file26919line268>
bq. >
bq. > why boxed longs?
Changed to AtomicLong[]
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java, line
369
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26920#file26920line369>
bq. >
bq. > for consistency, I think we should name these something like:
bq. > hfile.block.cache.offheap.size
bq. > hfile.block.cache.offheap.minblocksize
bq. >
bq. > also, is min block size really a min? isn't it more like
expectedblocksize?
Will do next revision.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java,
line 84
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26921#file26921line84>
bq. >
bq. > nowhere in this test does it actually verify that the data is valid
Fixed.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java,
line 38
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26921#file26921line38>
bq. >
bq. > should use JUnit 4 style tests in new code - i.e not inherit
TestCase.
I thought since everything else was JUnit 3 style, I should leave it as JUnit 3
style.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/test/java/org/apache/hadoop/hbase/io/hfile/TestSlabCache.java, line
109
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26922#file26922line109>
bq. >
bq. > see above.
bq. >
bq. > If you can refactor some of this code into something like
BlockCacheTestUtils.testBasicCacheContract(BlockCache cache) that would be
good... copy paste evil
Will fix this when I redo all tests tomorrow.
bq. On 2011-07-25 23:48:42, Todd Lipcon wrote:
bq. > src/test/java/org/apache/hadoop/hbase/io/hfile/TestSingleSlabCache.java,
line 95
bq. > <https://reviews.apache.org/r/1191/diff/1/?file=26921#file26921line95>
bq. >
bq. > 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
Will fix this when I redo all tests tomorrow.
- Li
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1191/#review1182
-----------------------------------------------------------
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
