wchevreuil commented on code in PR #5866:
URL: https://github.com/apache/hbase/pull/5866#discussion_r1593769305
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileWriter.java:
##########
Review Comment:
Let's minimize the number of line changes. We don't need all these
refactoring moving TimeRangeTracker update logic from StoreFileWriter to
HFileWriterImpl if we just pass the same TimeRangeTracker instance to
HFileWriterImpl.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterImpl.java:
##########
@@ -555,16 +563,33 @@ private void writeInlineBlocks(boolean closing) throws
IOException {
private void doCacheOnWrite(long offset) {
cacheConf.getBlockCache().ifPresent(cache -> {
HFileBlock cacheFormatBlock = blockWriter.getBlockForCaching(cacheConf);
+ BlockCacheKey key = buildBlockCacheKey(offset, cacheFormatBlock);
+ if (!shouldCacheBlock(cache, key)) {
Review Comment:
Yeah, the work we do inside DataTieringManager to figure out the store
config is a problem, the way it is implemented here, we will be doing for every
block, even though the config is exactly the same. Worse, we already have that
info passed before to HFileWriterImpl. StoreFileWriter already has the HStore
config and this is passed along to HFileWriterImpl at initialization time. We
should keep this config as global variable in HFileWriterImpl. Then we should
change the added shouldCacheBlock method in the BlockCache API to accept the
config as parameter, or maybe wrap it with the key itself.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DataTieringManager.java:
##########
@@ -231,10 +256,12 @@ public Set<String> getColdDataFiles(Set<BlockCacheKey>
allCachedBlocks)
}
private HRegion getHRegion(Path hFilePath) throws DataTieringException {
- if (hFilePath.getParent() == null || hFilePath.getParent().getParent() ==
null) {
- throw new DataTieringException("Incorrect HFile Path: " + hFilePath);
+ String regionId;
+ try {
+ regionId = HRegionFileSystem.getRegionId(hFilePath);
+ } catch (IOException e) {
+ throw new DataTieringException(e.getMessage());
}
Review Comment:
Nice code reuse!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
