[
https://issues.apache.org/jira/browse/HBASE-4422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13120543#comment-13120543
]
[email protected] commented on HBASE-4422:
------------------------------------------------------
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. >
/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractHFileReader.java, line
170
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47785#file47785line170>
bq. >
bq. > is .toString() necessary?
removed
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java, line 35
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47787#file47787line35>
bq. >
bq. > Why not make this private?
done
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java, line 67
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47787#file47787line67>
bq. >
bq. > Should this be suffixed with _KEY for consistency with other conf
key constants in this class?
done
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java, line
169
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47787#file47787line169>
bq. >
bq. > 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.
it actually has to be used if you want to manually set stuff, so it does get
used inside of some unit tests...
It's actually only used from within the package so I will make it pkg private
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java, line
268
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47787#file47787line268>
bq. >
bq. > Why is this not private?
There is a RandomSeek unit test that injects a block cache. This is not a
change in behavior (moved over from how it was in StoreFile previously)
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java, line
307
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47787#file47787line307>
bq. >
bq. > nit: space between "if" and the parenthesis :)
yeah i do that a lot :) changed... thanks for the detailed look!
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV1.java, line
355
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47790#file47790line355>
bq. >
bq. > Will this always return false if there is no block cache?
yes. all of the cacheConf calls do:
public boolean shouldEvictOnClose() {
return isBlockCacheEnabled() && this.evictOnClose;
}
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java, line
244
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47791#file47791line244>
bq. >
bq. > cacheBlock should probably be set to false if
!cacheBlock.isBlockCacheEnabled()
same thing as above. shouldCacheDataOnRead() is false if block cache is not
enabled.
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java, line
265
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47791#file47791line265>
bq. >
bq. > cacheBlock is not set to false if !cacheConf.isBlockCacheEnabled().
Could this result in an NPE?
same as above. no check from cacheConf ever returns true if block cache is
globally disabled
bq. On 2011-10-04 21:28:58, Mikhail Bautin wrote:
bq. > /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java, line
168
bq. > <https://reviews.apache.org/r/2089/diff/5/?file=47793#file47793line168>
bq. >
bq. > It would be good for readability and maybe for performance to assign
cacheConf.shouldCacheIndexesOnWrite() to a local variable.
done
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2089/#review2318
-----------------------------------------------------------
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
