joshelser commented on a change in pull request #3389: URL: https://github.com/apache/hbase/pull/3389#discussion_r671950730
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DirectStoreCompactor.java ########## @@ -0,0 +1,100 @@ +/** + * 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; + +/** + * Alternative Compactor implementation, this class extends <code>DefaultCompactor</code> class, Review comment: nit: First sentence should tell us what this class does "Places the compacted file directly into the store without the need for a rename" (and then tell us implementation details). ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestDirectStoreCompactor.java ########## @@ -0,0 +1,135 @@ +/** + * 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 static junit.framework.TestCase.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.hbase.CellComparator; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.io.ByteBuffAllocator; +import org.apache.hadoop.hbase.io.compress.Compression; +import org.apache.hadoop.hbase.io.crypto.Encryption; +import org.apache.hadoop.hbase.io.hfile.CacheConfig; +import org.apache.hadoop.hbase.io.hfile.HFileContext; +import org.apache.hadoop.hbase.regionserver.BloomType; +import org.apache.hadoop.hbase.regionserver.HRegion; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.regionserver.HStore; +import org.apache.hadoop.hbase.regionserver.HStoreFile; +import org.apache.hadoop.hbase.regionserver.StoreContext; +import org.apache.hadoop.hbase.regionserver.StoreFileWriter; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.util.ChecksumType; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; +import org.mockito.ArgumentCaptor; + +/** + * Test class for DirectStoreCompactor. + */ +@Category({ RegionServerTests.class, MediumTests.class }) +public class TestDirectStoreCompactor { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestDirectStoreCompactor.class); + + @Rule + public TestName name = new TestName(); + + private Configuration config = new Configuration(); + private HStore mockStore; + private String cfName = name.getMethodName()+"-CF"; + private Compactor.FileDetails mockFileDetails; + + @Before + public void setup() throws Exception { + Path filePath = new Path(name.getMethodName()); + mockStore = mock(HStore.class); + HRegionFileSystem mockRegionFS = mock(HRegionFileSystem.class); + when(mockStore.getRegionFileSystem()).thenReturn(mockRegionFS); + when(mockRegionFS.getRegionDir()).thenReturn(filePath); + when(mockStore.getColumnFamilyName()).thenReturn(cfName); + HFileContext mockFileContext = mock(HFileContext.class); Review comment: What about making a real `HFileContext` to avoid ~5 mockings? ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DirectStoreCompactor.java ########## @@ -0,0 +1,100 @@ +/** + * 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; + +/** + * Alternative Compactor implementation, this class extends <code>DefaultCompactor</code> class, + * modifying original behaviour of <code>initWriter</code> and <code>createFileInStoreDir</code> + * methods to create compacted files in the final store directory, rather than temp, avoid the + * need to perform renames at compaction commit time. + */ [email protected] +public class DirectStoreCompactor extends DefaultCompactor { + public DirectStoreCompactor(Configuration conf, HStore store) { + super(conf, store); + } + + /** + * Overrides <code>Compactor</code> original implementation to create the resulting file directly + * in the store dir, rather than temp, in order to avoid the need for rename at commit time. + * @param fd the file details. + * @param shouldDropBehind boolean for the drop-behind output stream cache settings. + * @param major if compaction is major. + * @return + * @throws IOException + */ + @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 createWriterInStoreDir(fd.maxKeyCount, + major ? majorCompactionCompression : minorCompactionCompression, + fd.maxMVCCReadpoint > 0, fd.maxTagsLength > 0, + shouldDropBehind, fd.totalCompactedFilesSize); + } + + private StoreFileWriter createWriterInStoreDir(long maxKeyCount, + Compression.Algorithm compression, boolean includeMVCCReadpoint, boolean includesTag, + boolean shouldDropBehind, long totalCompactedFilesSize) throws IOException { + final CacheConfig writerCacheConf; + writerCacheConf = new CacheConfig(store.getCacheConfig()); Review comment: one line? ########## 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: I keep trying to think about what the right abstraction to have for our internal API. Is there a reason to not have the pluggable compaction code have a method like this to return the HStoreFile? Glancing, I don't see any obvious methods/classes which are package-private to HStore (that would require these methods to live here). Although, I also see that `HStore#openStoreFiles` is using one instance of `createStoreFileAndReader`, so we couldn't completely move this out of HStore. Is it better to lift one method into `Store` and leave other instances "internal" to HStore? That might help the smell a little, but an obvious solution isn't jumping out at me. ########## 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: Same way above, I think it makes sense to not have the abstract Compactor dealing with _where_ storefiles are put as a part of the compaction algorithm. That's an implementation detail of the algorithm itself. Can we reasonably push down where paths/files are stored into the implementation itself? ########## 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: Actually, I guess this might make sense to move to StoreEngine if multiple things need it? (rather than my above suggestion) ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java ########## @@ -1930,4 +1959,21 @@ public boolean add(T e) { @Override public List<T> subList(int fromIndex, int toIndex) {return delegatee.subList(fromIndex, toIndex);} } + + private static class DummyCompactor extends DefaultCompactor { + + static CountDownLatch countDownLatch; + + public DummyCompactor(Configuration conf, HStore store) { Review comment: nit: if the Compactor signature isn't important, could pass the CountDownLatch in here. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestDefaultCompactor.java ########## @@ -0,0 +1,123 @@ +/** + * 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 static junit.framework.TestCase.assertEquals; +import static org.mockito.Mockito.mock; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.regionserver.HStore; +import org.apache.hadoop.hbase.regionserver.HStoreFile; +import org.apache.hadoop.hbase.regionserver.StoreFileWriter; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.After; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; + +/** + * Test class for DefaultCompactor. + */ +@Category({ RegionServerTests.class, MediumTests.class }) +public class TestDefaultCompactor { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestDefaultCompactor.class); + + @Rule + public TestName name = new TestName(); + + private final Configuration config = new Configuration(); + private HStore store; + private final String cfName = "cf"; + private Compactor.FileDetails mockFileDetails; + + private HBaseTestingUtility UTIL = new HBaseTestingUtility(); + private TableName table; + + @Before + public void setup() throws Exception { Review comment: Can we do this once as a `static`? ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestDirectStoreCompactor.java ########## @@ -0,0 +1,135 @@ +/** + * 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 static junit.framework.TestCase.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.hbase.CellComparator; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.io.ByteBuffAllocator; +import org.apache.hadoop.hbase.io.compress.Compression; +import org.apache.hadoop.hbase.io.crypto.Encryption; +import org.apache.hadoop.hbase.io.hfile.CacheConfig; +import org.apache.hadoop.hbase.io.hfile.HFileContext; +import org.apache.hadoop.hbase.regionserver.BloomType; +import org.apache.hadoop.hbase.regionserver.HRegion; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.regionserver.HStore; +import org.apache.hadoop.hbase.regionserver.HStoreFile; +import org.apache.hadoop.hbase.regionserver.StoreContext; +import org.apache.hadoop.hbase.regionserver.StoreFileWriter; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.util.ChecksumType; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; +import org.mockito.ArgumentCaptor; + +/** + * Test class for DirectStoreCompactor. + */ +@Category({ RegionServerTests.class, MediumTests.class }) +public class TestDirectStoreCompactor { Review comment: Is there a reason for all the mocking here compared to the TestDefaultCompactor? The mocking is pretty intense right now, which is usually a smell. This early on, it might be worthwhile to get some solid testing api. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestDirectStoreCompactor.java ########## @@ -0,0 +1,135 @@ +/** + * 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 static junit.framework.TestCase.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.hbase.CellComparator; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.io.ByteBuffAllocator; +import org.apache.hadoop.hbase.io.compress.Compression; +import org.apache.hadoop.hbase.io.crypto.Encryption; +import org.apache.hadoop.hbase.io.hfile.CacheConfig; +import org.apache.hadoop.hbase.io.hfile.HFileContext; +import org.apache.hadoop.hbase.regionserver.BloomType; +import org.apache.hadoop.hbase.regionserver.HRegion; +import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; +import org.apache.hadoop.hbase.regionserver.HStore; +import org.apache.hadoop.hbase.regionserver.HStoreFile; +import org.apache.hadoop.hbase.regionserver.StoreContext; +import org.apache.hadoop.hbase.regionserver.StoreFileWriter; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.util.ChecksumType; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; +import org.mockito.ArgumentCaptor; + +/** + * Test class for DirectStoreCompactor. + */ +@Category({ RegionServerTests.class, MediumTests.class }) +public class TestDirectStoreCompactor { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestDirectStoreCompactor.class); + + @Rule + public TestName name = new TestName(); + + private Configuration config = new Configuration(); + private HStore mockStore; + private String cfName = name.getMethodName()+"-CF"; + private Compactor.FileDetails mockFileDetails; + + @Before + public void setup() throws Exception { + Path filePath = new Path(name.getMethodName()); + mockStore = mock(HStore.class); + HRegionFileSystem mockRegionFS = mock(HRegionFileSystem.class); + when(mockStore.getRegionFileSystem()).thenReturn(mockRegionFS); + when(mockRegionFS.getRegionDir()).thenReturn(filePath); + when(mockStore.getColumnFamilyName()).thenReturn(cfName); + HFileContext mockFileContext = mock(HFileContext.class); + when(mockFileContext.getBytesPerChecksum()).thenReturn(100); + when(mockStore.createFileContext(isNull(), anyBoolean(), + anyBoolean(), isNull())).thenReturn(mockFileContext); + when(mockStore.getHRegion()).thenReturn(mock(HRegion.class)); + when(mockStore.getStoreContext()).thenReturn(new StoreContext.Builder(). + withFavoredNodesSupplier(()-> null).build()); + ColumnFamilyDescriptor mockDesc = mock(ColumnFamilyDescriptor.class); Review comment: Same here: real CFD? ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java ########## @@ -1731,6 +1733,33 @@ public void testHFileContextSetWithCFAndTable() throws Exception { assertArrayEquals(table, hFileContext.getTableName()); } + @Test + public void testDoCompactionDelegatesCommit() throws Exception { + final Configuration conf = HBaseConfiguration.create(TEST_UTIL.getConfiguration()); + conf.set(DEFAULT_COMPACTOR_CLASS_KEY, DummyCompactor.class.getName()); + HFileContext fileContext = new HFileContextBuilder().withBlockSize(BLOCKSIZE_SMALL).build(); + store = init(this.name.getMethodName(), conf); + Path storeDir =this.store.getStoreContext().getRegionFileSystem().getStoreDir("family"); + StoreFileWriter w = new StoreFileWriter.Builder(conf, new CacheConfig(conf), + store.getFileSystem()) + .withOutputDir(storeDir) + .withFileContext(fileContext) + .build(); + w.appendMetadata(1, false); + w.close(); + HStoreFile mockStoreFile = mock(HStoreFile.class); + List<HStoreFile> mockFiles = new ArrayList<>(); + mockFiles.add(mockStoreFile); + List<Path> files = new ArrayList<>(); + files.add(w.getPath()); + DummyCompactor.countDownLatch = new CountDownLatch(1); + try { + store.doCompaction(mock(CompactionRequestImpl.class), + null, mock(User.class), 0, files); Review comment: `fail()` within `try` ########## 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: Looks like you did this? Resolve conversation if so, please. ########## 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: > re-implement this in the DirectStoreCompactor I feel like that would just result in having to change code in multiple places which is error-prone. What about moving this onto `HFileContext` itself, or the `Builder`. Create a method which lets you pass the `Builder` an `Store` and do most of this work there. That seems to be more appropriate to me (at a glance) than doing this in HStore. ########## 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: Would recommend making sure the Javadoc is accurate and correcting the method name since you're moving the method. Can always leave this old name as "deprecated" which calls a better-named version, if you think it would help alleviate confusion. ########## 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: suggestion: leave the variable private, make a public (if necessary) getter for this method, and make a package-private/private method to mutate the value. ########## 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 creating them in the temp directory really "default"? Or is it "default" just because the direct-insert stuff isn't available yet? ;) When you say other compactors may not use it, it makes me wonder if it makes sense to provide it here. -- 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]
