[ https://issues.apache.org/jira/browse/HBASE-4422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13120490#comment-13120490 ]
jirapos...@reviews.apache.org commented on HBASE-4422: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2089/#review2318 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java <https://reviews.apache.org/r/2089/#comment5323> is .toString() necessary? /src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileWriter.java <https://reviews.apache.org/r/2089/#comment5324> This simplifies things a lot. /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java <https://reviews.apache.org/r/2089/#comment5325> Why not make this private? /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java <https://reviews.apache.org/r/2089/#comment5326> Should this be suffixed with _KEY for consistency with other conf key constants in this class? /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java <https://reviews.apache.org/r/2089/#comment5327> Is this constructor ever used outside of CacheConfig itself? It looks like a potential source of errors, because the caller has to get the order of 7 boolean arguments right. /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java <https://reviews.apache.org/r/2089/#comment5328> Why is this not private? /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java <https://reviews.apache.org/r/2089/#comment5329> nit: space between "if" and the parenthesis :) /src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java <https://reviews.apache.org/r/2089/#comment5330> This is really simplifying a lot of code :) /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java <https://reviews.apache.org/r/2089/#comment5331> It might be worth it to rename isInMemory() to useInMemoryCacheBucket() or isInMemoryCF(). The current name is too general. /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java <https://reviews.apache.org/r/2089/#comment5332> Will this always return false if there is no block cache? /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java <https://reviews.apache.org/r/2089/#comment5334> cacheBlock should probably be set to false if !cacheBlock.isBlockCacheEnabled() /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java <https://reviews.apache.org/r/2089/#comment5333> cacheBlock is not set to false if !cacheConf.isBlockCacheEnabled(). Could this result in an NPE? /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java <https://reviews.apache.org/r/2089/#comment5335> It would be good for readability and maybe for performance to assign cacheConf.shouldCacheIndexesOnWrite() to a local variable. - Mikhail 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