wchevreuil commented on a change in pull request #3389:
URL: https://github.com/apache/hbase/pull/3389#discussion_r663780661
##########
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:
Yeah, this logic was moved here from `HStore.createWriterInTmp` (as you
noticed later on your following comment). Because now we override the writer
creation in DirectStoreCompactor to not use this `HStore.createWriterInTmp`, we
would need to duplicate this logic there, but since `CacheConfig` already has
all the info required to decide on this, thought about moving it to
`CacheConfig`.
I reckon naming and cohesion is not been great, but I was just trying to
concentrate on the original problem here of allowing compacting files getting
created in the store directory, without the need for tmp dirs and renames.
##########
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:
BTW, let me fix the javadoc description.
##########
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:
Thanks!
##########
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:
Yep, see my reply above.
##########
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:
Yeah, should be only `enableCacheOnWrite`, as here we are calling it for
other writes than compactions.
##########
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:
BTW, let me fix the javadoc description.
##########
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:
Not needed after the changes addressing some of @z-york suggestions. Let
me revert 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:
Honestly, I can't remember now why I had duplicated `cryptoContext` on
`HStore` here, when it's already defined on `StoreContext`. It doesn't seem
correct to me now, let me remove. The need to expose `getStoreContext` as
`public` stands, though, as we need that from the
`DirectStoreCompactor.createWriterInFamilyDir`, which is in a separate package.
##########
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:
Prior tho this, the "commit compaction" logic was implemented straight
inside `HStore.doCompaction -> moveCompactedFilesIntoPlace ->
moveFileIntoPlace` which was where `createStoreFileAndReader` was getting
called. I believe this is "compaction specific" logic, and if we are aiming for
pluggable compaction behaviour, it shouldn't be implemented in the HStore.
That's why here we changed it to be `HStore.doCompaction ->
storeEngine.compactor.commitCompaction`, so we had to eventually call it from
the compactors.
##########
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:
This one in particular, could had logic duplicated wherever it's needed,
but it's more of a helper method, so didn't think making it public would be
harmful. I can revert it back to package private and re-implement this in the
`DirectStoreCompactor`.
##########
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:
This one in particular, could had logic duplicated wherever it's needed,
but it's more of a helper method, so didn't think making it public would be
harmful. I can revert it back to package private and re-implement this in the
`DirectStoreCompactor`, if you think this shouldn't be exposed.
##########
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:
This was the only marked as private here. Unfortunately needed exposing
that to `DirectStoreCompactor`.
##########
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:
Noted. Removing 'this' on next commit.
##########
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:
StoreEngine delegates the work to a `Compactor` implementation, where
`DefaultCompator`, the default Compactor implementation, uses this one to
create the file writer. So that's why we say it's the "default" method.
Alternatives compactors may not use it, like the `DirectStoreCompactor` that
implements its own logic that creates the file on the store directory, rather
than under temp.
##########
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:
We still need to vary the logic of "where" the store file is originally
created. Default behaviour is to create it under temp (that's done by the call
from lines #591/#592), whilst `DirectStoreCompactor` assumes the passed path is
already in the store dir itself, so it just needs to create the reader over the
passed path.
##########
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:
Will do.
##########
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:
Will do.
--
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]
