This is an automated email from the ASF dual-hosted git repository. apurtell pushed a commit to branch branch-2.5 in repository https://gitbox.apache.org/repos/asf/hbase.git
commit 94357694201d3e221aee69980174b88d4f153c65 Author: Duo Zhang <[email protected]> AuthorDate: Wed Jan 19 13:59:35 2022 +0800 HBASE-26674 Should modify filesCompacting under storeWriteLock (#4040) Signed-off-by: Josh Elser <[email protected]> --- .../main/java/org/apache/hadoop/hbase/regionserver/HStore.java | 9 +++++---- .../java/org/apache/hadoop/hbase/regionserver/StoreEngine.java | 6 ++++-- .../java/org/apache/hadoop/hbase/regionserver/TestHStore.java | 4 ++-- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java index 9016156..0c214e9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java @@ -1226,13 +1226,14 @@ public class HStore implements Store, HeapSize, StoreConfigInformation, allowedOnPath = ".*/(HStore|TestHStore).java") void replaceStoreFiles(Collection<HStoreFile> compactedFiles, Collection<HStoreFile> result, boolean writeCompactionMarker) throws IOException { - storeEngine.replaceStoreFiles(compactedFiles, result); + storeEngine.replaceStoreFiles(compactedFiles, result, () -> { + synchronized(filesCompacting) { + filesCompacting.removeAll(compactedFiles); + } + }); if (writeCompactionMarker) { writeCompactionWalRecord(compactedFiles, result); } - synchronized (filesCompacting) { - filesCompacting.removeAll(compactedFiles); - } // These may be null when the RS is shutting down. The space quota Chores will fix the Region // sizes later so it's not super-critical if we miss these. RegionServerServices rsServices = region.getRegionServerServices(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java index ddb52d1..d85553a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreEngine.java @@ -410,7 +410,8 @@ public abstract class StoreEngine<SF extends StoreFlusher, CP extends Compaction List<HStoreFile> openedFiles = openStoreFiles(toBeAddedFiles, false); // propogate the file changes to the underlying store file manager - replaceStoreFiles(toBeRemovedStoreFiles, openedFiles); // won't throw an exception + replaceStoreFiles(toBeRemovedStoreFiles, openedFiles, () -> { + }); // won't throw an exception } /** @@ -493,12 +494,13 @@ public abstract class StoreEngine<SF extends StoreFlusher, CP extends Compaction } public void replaceStoreFiles(Collection<HStoreFile> compactedFiles, - Collection<HStoreFile> newFiles) throws IOException { + Collection<HStoreFile> newFiles, Runnable actionUnderLock) throws IOException { storeFileTracker.replace(StoreUtils.toStoreFileInfo(compactedFiles), StoreUtils.toStoreFileInfo(newFiles)); writeLock(); try { storeFileManager.addCompactionResults(compactedFiles, newFiles); + actionUnderLock.run(); } finally { writeUnlock(); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java index b4d3b8a..ea7ff61 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java @@ -1034,14 +1034,14 @@ public class TestHStore { // call first time after files changed spiedStoreEngine.refreshStoreFiles(); assertEquals(2, this.store.getStorefilesCount()); - verify(spiedStoreEngine, times(1)).replaceStoreFiles(any(), any()); + verify(spiedStoreEngine, times(1)).replaceStoreFiles(any(), any(), any()); // call second time spiedStoreEngine.refreshStoreFiles(); // ensure that replaceStoreFiles is not called, i.e, the times does not change, if files are not // refreshed, - verify(spiedStoreEngine, times(1)).replaceStoreFiles(any(), any()); + verify(spiedStoreEngine, times(1)).replaceStoreFiles(any(), any(), any()); } private long countMemStoreScanner(StoreScanner scanner) {
