[hbase] branch master updated: HBASE-26674 Should modify filesCompacting under storeWriteLock (#4040)

[zhangduo]

This is an automated email from the ASF dual-hosted git repository.

zhangduo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/master by this push:
     new d63ca4f  HBASE-26674 Should modify filesCompacting under 
storeWriteLock (#4040)
d63ca4f is described below

commit d63ca4febef93e71afdf5677987342a7a9c69049
Author: Duo Zhang <zhang...@apache.org>
AuthorDate: Wed Jan 19 13:59:35 2022 +0800

    HBASE-26674 Should modify filesCompacting under storeWriteLock (#4040)
    
    Signed-off-by: Josh Elser <els...@apache.org>
---
 .../java/org/apache/hadoop/hbase/regionserver/HStore.java     | 11 ++++++-----
 .../org/apache/hadoop/hbase/regionserver/StoreEngine.java     |  6 ++++--
 .../java/org/apache/hadoop/hbase/regionserver/TestHStore.java |  4 ++--
 3 files changed, 12 insertions(+), 9 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 851257d..7301828 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
@@ -1230,13 +1230,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();
@@ -1567,7 +1568,7 @@ public class HStore implements Store, HeapSize, 
StoreConfigInformation,
     finishCompactionRequest(compaction.getRequest());
   }
 
-  protected void finishCompactionRequest(CompactionRequestImpl cr) {
+  private void finishCompactionRequest(CompactionRequestImpl cr) {
     this.region.reportCompactionRequestEnd(cr.isMajor(), cr.getFiles().size(), 
cr.getSize());
     if (cr.isOffPeak()) {
       offPeakCompactionTracker.set(false);
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 f6d58aa..7cc8193 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
@@ -1033,14 +1033,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) {