saintstack commented on a change in pull request #3389:
URL: https://github.com/apache/hbase/pull/3389#discussion_r660167381
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java
##########
@@ -291,12 +291,36 @@ public void setCacheDataOnWrite(boolean cacheDataOnWrite)
{
* cacheIndexesOnWrite
* cacheBloomsOnWrite
*/
- public void enableCacheOnWrite() {
+ public void enableCacheOnWriteForCompactions() {
this.cacheDataOnWrite = true;
this.cacheIndexesOnWrite = true;
this.cacheBloomsOnWrite = true;
}
+ /**
+ * If hbase.rs.cachecompactedblocksonwrite configuration is set to true and
+ * 'totalCompactedFilesSize' is lower than
'cacheCompactedDataOnWriteThreshold',
+ * enables cache on write for below properties:
+ * - cacheDataOnWrite
+ * - cacheIndexesOnWrite
+ * - cacheBloomsOnWrite
+ *
+ * Otherwise, sets 'cacheDataOnWrite' only to false.
+ *
+ * @param totalCompactedFilesSize the total size of compacted files.
+ * @return true if the checks mentioned above pass and the cache is enabled,
false otherwise.
+ */
+ public boolean enableCacheOnWriteForCompactions(long
totalCompactedFilesSize) {
Review comment:
Why the threshold check?
In description it talks of cacheCompactedDataOnWriteThreshold but the method
we call is getCacheCompactedBlocksOnWriteThreshold (and config is
cachecompactedblocksonwrite) and we check the return against
totalCompactedFilesSize. Its a little confusing on what is being compared here.
Any chance of some method renames or method name alignment w/ configs?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
##########
@@ -533,4 +550,46 @@ protected InternalScanner createScanner(HStore store,
ScanInfo scanInfo,
return new StoreScanner(store, scanInfo, scanners, smallestReadPoint,
earliestPutTs,
dropDeletesFromRow, dropDeletesToRow);
}
+
+ /**
+ * Default implementation for committing store files created after a
compaction. Assumes new files
+ * had been created on a temp directory, so it renames those files into the
actual store dir,
+ * then create a reader and cache it into the store.
+ * @param cr the compaction request.
+ * @param newFiles the new files created by this compaction under a temp dir.
+ * @param user the running user.
+ * @return A list of the resulting store files already placed in the store
dir and loaded into the
+ * store cache.
+ * @throws IOException if the commit fails.
+ */
+ public List<HStoreFile> commitCompaction(CompactionRequestImpl cr,
List<Path> newFiles, User user)
+ throws IOException {
+ List<HStoreFile> sfs = new ArrayList<>(newFiles.size());
+ for (Path newFile : newFiles) {
+ assert newFile != null;
+ this.store.validateStoreFile(newFile);
+ // Move the file into the right spot
+ HStoreFile sf = createFileInStoreDir(newFile);
+ if (this.store.getCoprocessorHost() != null) {
+ this.store.getCoprocessorHost().postCompact(this.store, sf,
cr.getTracker(), cr, user);
+ }
+ assert sf != null;
+ sfs.add(sf);
+ }
+ return sfs;
+ }
+
+ /**
+ * Assumes new file was created initially on a temp folder.
Review comment:
Don't we want the Compactor asking the StoreEngine to do the writing for
us? Rather than doing it here?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DirectStoreCompactor.java
##########
@@ -0,0 +1,85 @@
+/**
+ * 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.compactions;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.io.compress.Compression;
+import org.apache.hadoop.hbase.io.hfile.CacheConfig;
+import org.apache.hadoop.hbase.io.hfile.HFileContext;
+import org.apache.hadoop.hbase.regionserver.HStore;
+import org.apache.hadoop.hbase.regionserver.HStoreFile;
+import org.apache.hadoop.hbase.regionserver.StoreFileWriter;
+import org.apache.yetus.audience.InterfaceAudience;
+
[email protected]
+public class DirectStoreCompactor extends DefaultCompactor {
+ public DirectStoreCompactor(Configuration conf, HStore store) {
+ super(conf, store);
+ }
+
+ @Override
+ protected StoreFileWriter initWriter(FileDetails fd, boolean
shouldDropBehind, boolean major)
+ throws IOException {
+ // When all MVCC readpoints are 0, don't write them.
+ // See HBASE-8166, HBASE-12600, and HBASE-13389.
+ return createWriterInFamilyDir(fd.maxKeyCount,
+ major ? majorCompactionCompression : minorCompactionCompression,
+ fd.maxMVCCReadpoint > 0, fd.maxTagsLength > 0,
+ shouldDropBehind, fd.totalCompactedFilesSize);
+ }
+
+ private StoreFileWriter createWriterInFamilyDir(long maxKeyCount,
Review comment:
In Compactor we have createFileInStoreDir but here its
createWriterInFamilyDir... why not createWriterInStoreDir
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -1372,7 +1372,7 @@ public RegionInfo getRegionInfo() {
* @return Instance of {@link RegionServerServices} used by this HRegion.
* Can be null.
*/
- RegionServerServices getRegionServerServices() {
+ public RegionServerServices getRegionServerServices() {
Review comment:
We have to make this public? And we have to get services here?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DirectStoreFlushContext.java
##########
@@ -30,14 +30,14 @@
* directly in the store dir, and therefore, doesn't perform a rename from tmp
dir
* into the store dir.
*
- * To be used only when PersistedStoreEngine is configured as the StoreEngine
implementation.
+ * To be used only when DirectStoreFlushContext is configured as the
StoreEngine implementation.
Review comment:
I like the rename.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
##########
@@ -278,6 +280,21 @@ protected final StoreFileWriter
createTmpWriter(FileDetails fd, boolean shouldDr
HConstants.EMPTY_STRING);
}
+ /**
+ * Default method for initializing a StoreFileWriter in the compaction
process, this creates the
+ * resulting files on a temp directory. Therefore, upon compaction commit
time, these files
+ * should be renamed into the actual store dir.
+ * @param fd the file details.
+ * @param shouldDropBehind boolean for the drop-behind output stream cache
settings.
+ * @param major if compaction is major.
+ * @return Writer for a new StoreFile in the tmp dir.
+ * @throws IOException if it fails to initialise the writer.
+ */
+ protected StoreFileWriter initWriter(FileDetails fd, boolean
shouldDropBehind, boolean major)
+ throws IOException {
+ return this.createTmpWriter(fd, shouldDropBehind, major);
Review comment:
Why the 'this'? Seems unorthodox? We don't usually do this?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -431,7 +437,7 @@ public static long determineTTLFromFamily(final
ColumnFamilyDescriptor family) {
return ttl;
}
- StoreContext getStoreContext() {
+ public StoreContext getStoreContext() {
Review comment:
Crypto context is table-scoped? We have to get it from the Store?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DirectStoreCompactor.java
##########
@@ -0,0 +1,85 @@
+/**
+ * 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.compactions;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.io.compress.Compression;
+import org.apache.hadoop.hbase.io.hfile.CacheConfig;
+import org.apache.hadoop.hbase.io.hfile.HFileContext;
+import org.apache.hadoop.hbase.regionserver.HStore;
+import org.apache.hadoop.hbase.regionserver.HStoreFile;
+import org.apache.hadoop.hbase.regionserver.StoreFileWriter;
+import org.apache.yetus.audience.InterfaceAudience;
+
[email protected]
+public class DirectStoreCompactor extends DefaultCompactor {
Review comment:
Class comment on what this does?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -694,7 +700,7 @@ private void
refreshStoreFilesInternal(Collection<StoreFileInfo> newFiles) throw
refreshStoreSizeAndTotalBytes();
}
- protected HStoreFile createStoreFileAndReader(final Path p) throws
IOException {
+ public HStoreFile createStoreFileAndReader(final Path p) throws IOException {
Review comment:
Who would be calling this outside of this package?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1192,7 +1195,7 @@ public StoreFileWriter createWriterInTmp(long
maxKeyCount, Compression.Algorithm
return builder.build();
}
- HFileContext createFileContext(Compression.Algorithm compression,
+ public HFileContext createFileContext(Compression.Algorithm compression,
Review comment:
We have to make all this stuff public?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
##########
@@ -135,7 +135,7 @@ public CompactionProgress getProgress() {
/** Min SeqId to keep during a major compaction **/
public long minSeqIdToKeep = 0;
/** Total size of the compacted files **/
- private long totalCompactedFilesSize = 0;
+ public long totalCompactedFilesSize = 0;
Review comment:
Opening up all this stuff smells.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
##########
@@ -278,6 +280,21 @@ protected final StoreFileWriter
createTmpWriter(FileDetails fd, boolean shouldDr
HConstants.EMPTY_STRING);
}
+ /**
+ * Default method for initializing a StoreFileWriter in the compaction
process, this creates the
Review comment:
Is this right if we have the StoreEngine in the mix? Its supposed to do
this right?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1145,16 +1151,13 @@ public StoreFileWriter createWriterInTmp(long
maxKeyCount, Compression.Algorithm
// if data blocks are to be cached on write
// during compaction, we should forcefully
// cache index and bloom blocks as well
- if (cacheCompactedBlocksOnWrite && totalCompactedFilesSize <= cacheConf
- .getCacheCompactedBlocksOnWriteThreshold()) {
- writerCacheConf.enableCacheOnWrite();
+ if
(writerCacheConf.enableCacheOnWriteForCompactions(totalCompactedFilesSize)) {
Review comment:
Hmm.... Looks like you were trying to clean up some old stuff... The
weird naming is not yours.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java
##########
@@ -291,12 +291,36 @@ public void setCacheDataOnWrite(boolean cacheDataOnWrite)
{
* cacheIndexesOnWrite
* cacheBloomsOnWrite
*/
- public void enableCacheOnWrite() {
+ public void enableCacheOnWriteForCompactions() {
this.cacheDataOnWrite = true;
this.cacheIndexesOnWrite = true;
this.cacheBloomsOnWrite = true;
}
+ /**
+ * If hbase.rs.cachecompactedblocksonwrite configuration is set to true and
+ * 'totalCompactedFilesSize' is lower than
'cacheCompactedDataOnWriteThreshold',
+ * enables cache on write for below properties:
+ * - cacheDataOnWrite
+ * - cacheIndexesOnWrite
+ * - cacheBloomsOnWrite
+ *
+ * Otherwise, sets 'cacheDataOnWrite' only to false.
+ *
+ * @param totalCompactedFilesSize the total size of compacted files.
+ * @return true if the checks mentioned above pass and the cache is enabled,
false otherwise.
+ */
+ public boolean enableCacheOnWriteForCompactions(long
totalCompactedFilesSize) {
Review comment:
Whats this threshold about? Why we have it? Is there a pointer to why?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -1167,7 +1170,7 @@ public StoreFileWriter createWriterInTmp(long
maxKeyCount, Compression.Algorithm
} else {
final boolean shouldCacheDataOnWrite =
cacheConf.shouldCacheDataOnWrite();
if (shouldCacheDataOnWrite) {
- writerCacheConf.enableCacheOnWrite();
+ writerCacheConf.enableCacheOnWriteForCompactions();
Review comment:
What if I want to cache on write when its other than compactions?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
