saintstack commented on a change in pull request #2800:
URL: https://github.com/apache/hbase/pull/2800#discussion_r552255807
##########
File path:
hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
##########
@@ -138,6 +138,10 @@ public boolean isCompressedOrEncrypted() {
return compressAlgo;
}
+ public void setCompression(Compression.Algorithm compressAlgo) {
+ this.compressAlgo = compressAlgo;
+ }
+
Review comment:
Whats going on here?
HFileContext is supposed to be ' Read-only HFile Context Information' but
here we are adding a setter. I see there are one or two setters but we also
have HFileContextBuilder. Shouldn't we be going via the Builder making
HFileContexts? And why is this method not added on the Builder? (Can it be
added non-public to encourage users to go via the Builder)?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -254,48 +244,47 @@
protected HStore(final HRegion region, final ColumnFamilyDescriptor family,
final Configuration confParam, boolean warmup) throws IOException {
- this.fs = region.getRegionFileSystem();
-
- // Assemble the store's home directory and Ensure it exists.
- fs.createStoreDir(family.getNameAsString());
- this.region = region;
- this.family = family;
// 'conf' renamed to 'confParam' b/c we use this.conf in the constructor
// CompoundConfiguration will look for keys in reverse order of addition,
so we'd
// add global config first, then table and cf overrides, then cf metadata.
this.conf = new CompoundConfiguration()
- .add(confParam)
- .addBytesMap(region.getTableDescriptor().getValues())
- .addStringMap(family.getConfiguration())
- .addBytesMap(family.getValues());
- this.blocksize = family.getBlocksize();
+ .add(confParam)
+ .addBytesMap(region.getTableDescriptor().getValues())
+ .addStringMap(family.getConfiguration())
+ .addBytesMap(family.getValues());
+
+ this.region = region;
+ this.storeContext = initializeStoreContext(family);
+
+ // Assemble the store's home directory and Ensure it exists.
+ getRegionFileSystem().createStoreDir(getColumnFamilyName());
+
+ this.blocksize = getColumnFamilyDescriptor().getBlocksize();
// set block storage policy for store directory
- String policyName = family.getStoragePolicy();
+ String policyName = getColumnFamilyDescriptor().getStoragePolicy();
Review comment:
nit: why bother with this change at all? 'family' is passed on the
constructor. Its final. Why bother going via the accessor in the constructor?
##########
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:
I would like to know here in class comment if this is
read-only/immutable. I think it should be.
##########
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:
Also, just a comment... the HFileContext has this for a comment...
* Read-only HFile Context Information. Meta data that is used by
HFileWriter/Readers and by
* HFileBlocks. Create one using the {@link HFileContextBuilder} (See
HFileInfo and the HFile
* Trailer class).
so it is for readers and writers.... So, StoreContext is probably fine but
if you are wondering... here is incidences of Info.java vs Context.java.... If
it helps.
```
kalashnikov:hbase.apache.git stack$ find src/main/java -name *Info.java
find: src/main/java: No such file or directory
kalashnikov:hbase.apache.git stack$ find hbase-*/src/main/java -name
*Info.java
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupTableInfo.java
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/security/SecurityInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/client/MutableRegionInfo.java
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RegionInfo.java
hbase-common/src/main/java/org/apache/hadoop/hbase/util/VersionInfo.java
hbase-common/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfo.java
hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/mode/DrillDownInfo.java
hbase-hbtop/src/main/java/org/apache/hadoop/hbase/hbtop/field/FieldInfo.java
hbase-metrics-api/src/main/java/org/apache/hadoop/hbase/metrics/MetricRegistryInfo.java
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationQueueInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HbckTableInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/HbckRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockWithScanInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/namespace/NamespaceTableAndRegionInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/master/webapp/RegionReplicaInfo.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallQueueInfo.java
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/generated/THRegionInfo.java
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift/generated/TRegionInfo.java
```
```
kalashnikov:hbase.apache.git stack$ find hbase-*/src/main/java -name
*Context.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Context.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileContext.java
hbase-common/src/main/java/org/apache/hadoop/hbase/io/TagCompressionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowPrefixFixedLengthBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/RowColBloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/util/BloomContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/ReaderContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/CompactionContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlushContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcSchedulerContext.java
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -347,6 +331,48 @@ protected HStore(final HRegion region, final
ColumnFamilyDescriptor family,
cacheOnWriteLogged = false;
}
+ private StoreContext initializeStoreContext(ColumnFamilyDescriptor family)
throws IOException {
+ return new StoreContext.Builder()
+ .withBloomType(family.getBloomFilterType())
+ .withCacheConfig(createCacheConf(family))
+ .withCellComparator(region.getCellComparator())
+ .withColumnFamilyDescriptor(family)
+ .withCompactedFilesSupplier(this::getCompactedFiles)
+ .withRegionFileSystem(region.getRegionFileSystem())
+ .withDefaultHFileContext(getDefaultHFileContext(family))
+ .withFavoredNodesSupplier(this::getFavoredNodes)
+ .withFamilyStoreDirectoryPath(region.getRegionFileSystem()
+ .getStoreDir(family.getNameAsString()))
+ .withRegionCoprocessorHost(region.getCoprocessorHost())
+ .build();
+ }
+
+ private InetSocketAddress[] getFavoredNodes() {
+ InetSocketAddress[] favoredNodes = null;
+ if (region.getRegionServerServices() != null) {
+ favoredNodes = region.getRegionServerServices().getFavoredNodesForRegion(
+ region.getRegionInfo().getEncodedName());
+ }
+ return favoredNodes;
+ }
+
+ private HFileContext getDefaultHFileContext(ColumnFamilyDescriptor family)
throws IOException {
Review comment:
Could be static?
##########
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:
Why would we do this here and not as methods on the builder?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -474,33 +502,14 @@ public long getBlockingFileCount() {
}
/* End implementation of StoreConfigInformation */
- /**
- * Returns the configured bytesPerChecksum value.
- * @param conf The configuration
- * @return The bytesPerChecksum that is set in the configuration
- */
- public static int getBytesPerChecksum(Configuration conf) {
- return conf.getInt(HConstants.BYTES_PER_CHECKSUM,
- HFile.DEFAULT_BYTES_PER_CHECKSUM);
- }
-
- /**
- * Returns the configured checksum algorithm.
- * @param conf The configuration
- * @return The checksum algorithm that is set in the configuration
- */
- public static ChecksumType getChecksumType(Configuration conf) {
- String checksumName = conf.get(HConstants.CHECKSUM_TYPE_NAME);
- if (checksumName == null) {
- return ChecksumType.getDefaultChecksumType();
- } else {
- return ChecksumType.nameToType(checksumName);
- }
- }
@Override
public ColumnFamilyDescriptor getColumnFamilyDescriptor() {
- return this.family;
+ return this.storeContext.getFamily();
+ }
+
+ public Encryption.Context getEncryptionContext() {
Review comment:
Has to be public? Has to be on HStore? Can it be on StoreContext? Is
there a getStoreContext method on HStore?
----------------------------------------------------------------
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
