> On 2010-11-30 09:57:27, Ryan Rawson wrote:
> > branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java,
> > line 765
> > <http://review.cloudera.org/r/1261/diff/1/?file=17902#file17902line765>
> >
> > why would you not want to evict blocks from the cache on close?
>
> stack wrote:
> I think this a good point. Its different behavior but its behavior we
> should have always had? One less option too.
>
> Ryan Rawson wrote:
> I'm still confused why we are adding config for something that we should
> always be doing it. While we'll never be zero conf, I am not seeing the
> reason why we'd want to keep things in the LRU.
>
> It would make more sense not to evict on a split, but evict every other
> time, since a split will probably reopen the same hfiles and need those
> blocks again.
I think it makes sense to have undocumented configuration parameters. The
default behavior is then "the way" but having a config option checked in the
code at least gives the opportunity to turn something on/off without making a
code change and redeploying completely. In the unit test, I'm turning it
on/off with the config parameter so I can verify it works as expected.
And although I've changed the default to true, I'm not convinced that it always
makes sense in all cases.
Ryan came up with example of the split, though that would override the config
parameter. But I think there could be other situations where you don't want to
as well.
In any case, I want to keep it configurable so I can turn it on/off between
test runs and see what, if any, difference these optimizations make and IMO
there's very little cost associated with using
conf.getBoolean("some.undocumented.thing", true) vs. a hard-coded true (if
there's any possibility you might want to change the behavior).
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1261/#review2010
-----------------------------------------------------------
On 2010-11-29 23:22:38, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1261/
> -----------------------------------------------------------
>
> (Updated 2010-11-29 23:22:38)
>
>
> Review request for hbase, stack and khemani.
>
>
> Summary
> -------
>
> This issue is about adding configuration options to add/remove from the block
> cache when creating/closing files. For use cases with lots of flushing and
> compacting, this might be desirable to prevent cache misses and maximize the
> effective utilization of total block cache capacity.
>
> The first option, hbase.rs.cacheblocksonwrite, will make it so we pre-cache
> blocks as we are writing out new files.
>
> The second option, hbase.rs.evictblocksonclose, will make it so we evict
> blocks when files are closed.
>
>
> This addresses bug HBASE-3287.
> http://issues.apache.org/jira/browse/HBASE-3287
>
>
> Diffs
> -----
>
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
> 1040422
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java
> 1040422
> branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java
> 1040422
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
> 1040422
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/io/hfile/SimpleBlockCache.java
> 1040422
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java
> 1040422
> branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
> 1040422
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
> 1040422
>
> branches/0.90/src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/HFilePerformanceEvaluation.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/RandomSeek.java
> 1040422
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFilePerformance.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileSeek.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestReseekTo.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/io/hfile/TestSeekTo.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFiles.java
> 1040422
>
> branches/0.90/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
> 1040422
>
> Diff: http://review.cloudera.org/r/1261/diff
>
>
> Testing
> -------
>
> Added a unit test to TestStoreFile. That passes.
>
> Need to do perf testing on a cluster.
>
>
> Thanks,
>
> Jonathan
>
>
