taklwu commented on a change in pull request #2800: URL: https://github.com/apache/hbase/pull/2800#discussion_r552320678
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreContext.java ########## @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.regionserver; + +import java.net.InetSocketAddress; +import java.util.Collection; +import java.util.function.Supplier; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.CellComparator; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.io.HeapSize; +import org.apache.hadoop.hbase.io.hfile.CacheConfig; +import org.apache.hadoop.hbase.io.hfile.HFileContext; +import org.apache.hadoop.hbase.util.ClassSize; +import org.apache.yetus.audience.InterfaceAudience; + +/** + * This carries the information on some of the meta data about the HStore. This + * meta data can be used across the HFileWriter/Readers and other HStore consumers without the + * need of passing around the complete store. Review comment: thanks for sharing a list of *Info and *Context, I browsed few of them and seems Context should be still okie. so let's keep it with `StoreContext` and made this change simple, and I add the `immutable` keyword in the block of the class comment. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java ########## @@ -1206,53 +1218,34 @@ public StoreFileWriter createWriterInTmp(long maxKeyCount, Compression.Algorithm } } } - InetSocketAddress[] favoredNodes = null; - if (region.getRegionServerServices() != null) { - favoredNodes = region.getRegionServerServices().getFavoredNodesForRegion( - region.getRegionInfo().getEncodedName()); - } - HFileContext hFileContext = createFileContext(compression, includeMVCCReadpoint, includesTag, - cryptoContext); - Path familyTempDir = new Path(fs.getTempDir(), family.getNameAsString()); - StoreFileWriter.Builder builder = new StoreFileWriter.Builder(conf, writerCacheConf, - this.getFileSystem()) - .withOutputDir(familyTempDir) - .withBloomType(family.getBloomFilterType()) - .withMaxKeyCount(maxKeyCount) - .withFavoredNodes(favoredNodes) - .withFileContext(hFileContext) - .withShouldDropCacheBehind(shouldDropBehind) - .withCompactedFilesSupplier(this::getCompactedFiles) - .withFileStoragePolicy(fileStoragePolicy); + HFileContext hFileContext = createFileContext(compression, includeMVCCReadpoint, includesTag); + Path familyTempDir = new Path(getRegionFileSystem().getTempDir(), getColumnFamilyName()); + StoreFileWriter.Builder builder = + new StoreFileWriter.Builder(conf, writerCacheConf, getFileSystem()) + .withOutputDir(familyTempDir) + .withBloomType(storeContext.getBloomFilterType()) + .withMaxKeyCount(maxKeyCount) + .withFavoredNodes(storeContext.getFavoredNodes()) + .withFileContext(hFileContext) + .withShouldDropCacheBehind(shouldDropBehind) + .withCompactedFilesSupplier(storeContext.getCompactedFilesSupplier()) + .withFileStoragePolicy(fileStoragePolicy); return builder.build(); } private HFileContext createFileContext(Compression.Algorithm compression, - boolean includeMVCCReadpoint, boolean includesTag, Encryption.Context cryptoContext) { + boolean includeMVCCReadpoint, boolean includesTag) { if (compression == null) { compression = HFile.DEFAULT_COMPRESSION_ALGORITHM; } - HFileContext hFileContext = new HFileContextBuilder() - .withIncludesMvcc(includeMVCCReadpoint) - .withIncludesTags(includesTag) - .withCompression(compression) - .withCompressTags(family.isCompressTags()) - .withChecksumType(checksumType) - .withBytesPerCheckSum(bytesPerChecksum) - .withBlockSize(blocksize) - .withHBaseCheckSum(true) - .withDataBlockEncoding(family.getDataBlockEncoding()) - .withEncryptionContext(cryptoContext) - .withCreateTime(EnvironmentEdgeManager.currentTime()) - .withColumnFamily(family.getName()) - .withTableName(region.getTableDescriptor() - .getTableName().getName()) - .withCellComparator(this.comparator) - .build(); - return hFileContext; + HFileContext fileContext = storeContext.getDefaultFileContext(); + fileContext.setIncludesMvcc(includeMVCCReadpoint); + fileContext.setIncludesTags(includesTag); + fileContext.setCompression(compression); + fileContext.setFileCreateTime(EnvironmentEdgeManager.currentTime()); Review comment: it was trying to reuse the `defaultFileContext` built with `initializeStoreContext`, but after rethinking on your comment, I'm going to remove the `defaultFileContext` and go back with the original builder pattern. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
