hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4422) Move block cache parameters and references into single CacheConf class
Date Tue, 04 Oct 2011 22:38:37 GMT

    [ https://issues.apache.org/jira/browse/HBASE-4422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13120544#comment-13120544
] 

jiraposter@reviews.apache.org commented on HBASE-4422:
------------------------------------------------------



bq.  On 2011-10-04 21:41:43, Mikhail Bautin wrote:
bq.  > Sending the second part of comments. Still have not reviewed StoreFile changes.
I guess my biggest concern is the initialization of a default CacheConfig in a lot of places
(hopefully mostly tests) while it could be initialized based on the available Configuration.

Yeah, it was a concern i had.  I can go through a remove all the empty constructors and require
a Configuration and see if I can still get all the tests to pass.


bq.  On 2011-10-04 21:41:43, Mikhail Bautin wrote:
bq.  > /src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java, line 113
bq.  > <https://reviews.apache.org/r/2089/diff/5/?file=47801#file47801line113>
bq.  >
bq.  >     Should not there be a way to create a default CacheConf() from a Configuration()?
Currently it seems that you are creating a default CacheConfig to pass to getWriteFactory
even though you have a conf available, potentially with some cache-related settings. Maybe
there should be an overload of HFile.getWriterFactory that takes a conf and creates a default
CacheConfig out of it.

I will go through and see if i can clean that up and not use an empty constructor of CacheConfig
anywhere


bq.  On 2011-10-04 21:41:43, Mikhail Bautin wrote:
bq.  > /src/test/java/org/apache/hadoop/hbase/io/hfile/RandomSeek.java, line 72
bq.  > <https://reviews.apache.org/r/2089/diff/5/?file=47804#file47804line72>
bq.  >
bq.  >     nit: trailing whitespace (we probably should remove all of it in one patch someday
:)

nipped


bq.  On 2011-10-04 21:41:43, Mikhail Bautin wrote:
bq.  > /src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java, line 185
bq.  > <https://reviews.apache.org/r/2089/diff/5/?file=47816#file47816line185>
bq.  >
bq.  >     Should not we move the cast to initialization and change blockCache's type to
LruBlockCache?
bq.  >     
bq.  >     Is this method only intended for testing, by the way? If so, should this be
indicated in the method name?

There was work done so we didn't need to have LruBlockCache and could use the BlockCache interface.
 Unfortunately the BlockCache interface doesn't expose a method to get the count of elements.
 I could just add it to the BlockCache interface?


bq.  On 2011-10-04 21:41:43, Mikhail Bautin wrote:
bq.  > /src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java, line 118
bq.  > <https://reviews.apache.org/r/2089/diff/5/?file=47805#file47805line118>
bq.  >
bq.  >     Do you want to keep this debug code around?

in a test, doesn't hurt i think (makes sense in context of this being a parameterized test)


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2089/#review2319
-----------------------------------------------------------


On 2011-10-04 02:50:58, Jonathan Gray wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/2089/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-10-04 02:50:58)
bq.  
bq.  
bq.  Review request for hbase, Dhruba Borthakur, Michael Stack, and Li Pi.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Creates a new CacheConfig class and moves almost everything block cache related into
this single class.  Adding new configuration params and booleans and such should be much better.
bq.  
bq.  All tests are NOT passing yet, still working on it, but wanted to have something up today.
 Basically "code complete" but broken :)
bq.  
bq.  
bq.  This addresses bug HBASE-4422.
bq.      https://issues.apache.org/jira/browse/HBASE-4422
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    /src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java PRE-CREATION 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV1.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/mapreduce/HFileOutputFormat.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java 1178676

bq.    /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/util/BloomFilterFactory.java 1178676 
bq.    /src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/HFilePerformanceEvaluation.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/RandomSeek.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFilePerformance.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileReaderV1.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileSeek.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileWriterV2.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestReseekTo.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/io/hfile/TestSeekTo.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java 1178676

bq.    /src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFiles.java 1178676

bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactSelection.java 1178676

bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java 1178676

bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/TestFSErrorsExposed.java 1178676

bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java 1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFileBlockCacheSummary.java
1178676 
bq.    /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java 1178676

bq.  
bq.  Diff: https://reviews.apache.org/r/2089/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  Still working through some tests that aren't passing.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Jonathan
bq.  
bq.


                
> Move block cache parameters and references into single CacheConf class
> ----------------------------------------------------------------------
>
>                 Key: HBASE-4422
>                 URL: https://issues.apache.org/jira/browse/HBASE-4422
>             Project: HBase
>          Issue Type: Improvement
>          Components: io
>            Reporter: Jonathan Gray
>            Assignee: Jonathan Gray
>             Fix For: 0.92.0
>
>
> From StoreFile down to HFile, we currently use a boolean argument for each of the various
block cache configuration parameters that exist.  The number of parameters is going to continue
to increase as we look at compressed cache, delta encoding, and more specific L1/L2 configuration.
 Every new config currently requires changing many constructors because it introduces a new
boolean.
> We should move everything into a single class so that modifications are much less disruptive.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message