[
https://issues.apache.org/jira/browse/HBASE-4422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13120501#comment-13120501
]
[email protected] commented on HBASE-4422:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2089/#review2319
-----------------------------------------------------------
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.
/src/main/java/org/apache/hadoop/hbase/util/CompressionTest.java
<https://reviews.apache.org/r/2089/#comment5336>
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.
/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
<https://reviews.apache.org/r/2089/#comment5337>
The same concern about default CacheConfig as above.
/src/test/java/org/apache/hadoop/hbase/io/hfile/RandomSeek.java
<https://reviews.apache.org/r/2089/#comment5338>
nit: trailing whitespace (we probably should remove all of it in one patch
someday :)
/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
<https://reviews.apache.org/r/2089/#comment5340>
Do you want to keep this debug code around?
/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java
<https://reviews.apache.org/r/2089/#comment5339>
Is this debug code?
/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
<https://reviews.apache.org/r/2089/#comment5341>
Should the CacheConfig be initialized based on the configuration? (the same
as other places)
/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileWriterV2.java
<https://reviews.apache.org/r/2089/#comment5343>
The same thing with CacheConfig and conf.
/src/test/java/org/apache/hadoop/hbase/io/hfile/TestReseekTo.java
<https://reviews.apache.org/r/2089/#comment5344>
The same thing with CacheConfig and conf.
/src/test/java/org/apache/hadoop/hbase/io/hfile/TestSeekTo.java
<https://reviews.apache.org/r/2089/#comment5345>
The same thing with CacheConfig and conf (OK, I won't mention this anymore).
/src/test/java/org/apache/hadoop/hbase/regionserver/TestBlocksRead.java
<https://reviews.apache.org/r/2089/#comment5348>
Should not we move the cast to initialization and change blockCache's type
to LruBlockCache?
Is this method only intended for testing, by the way? If so, should this be
indicated in the method name?
- 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
