[hbase] branch branch-2 updated: HBASE-26675 Data race on Compactor.writer (#4035)

[zh...@apache.org]

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

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


The following commit(s) were added to refs/heads/branch-2 by this push:
     new 5aa0fd2  HBASE-26675 Data race on Compactor.writer (#4035)
5aa0fd2 is described below

commit 5aa0fd2651b04dc251134ffbebb0fa59c3db01b6
Author: Duo Zhang <zh...@apache.org>
AuthorDate: Mon Jan 24 19:45:50 2022 +0800

    HBASE-26675 Data race on Compactor.writer (#4035)
    
    Signed-off-by: Xin Sun <dd...@gmail.com>
---
 .../hbase/regionserver/compactions/Compactor.java    | 20 +++++++++++---------
 .../regionserver/compactions/DefaultCompactor.java   | 12 +++++-------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
index a821a90..d934ecb 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/Compactor.java
@@ -95,7 +95,10 @@ public abstract class Compactor<T extends CellSink> {
   private final boolean dropCacheMajor;
   private final boolean dropCacheMinor;
 
-  protected T writer = null;
+  // In compaction process only a single thread will access and write to this field, and
+  // getCompactionTargets is the only place we will access it other than the compaction thread, so
+  // make it volatile.
+  protected volatile T writer = null;
 
   //TODO: depending on Store is not good but, realistically, all compactors currently do.
   Compactor(Configuration conf, HStore store) {
@@ -546,17 +549,16 @@ public abstract class Compactor<T extends CellSink> {
         dropDeletesFromRow, dropDeletesToRow);
   }
 
-  public List<Path> getCompactionTargets(){
-    if (writer == null){
+  public List<Path> getCompactionTargets() {
+    T writer = this.writer;
+    if (writer == null) {
       return Collections.emptyList();
     }
-    synchronized (writer){
-      if (writer instanceof StoreFileWriter){
-        return Arrays.asList(((StoreFileWriter)writer).getPath());
-      }
-      return ((AbstractMultiFileWriter)writer).writers().stream().map(sfw -> sfw.getPath()).collect(
-        Collectors.toList());
+    if (writer instanceof StoreFileWriter) {
+      return Arrays.asList(((StoreFileWriter) writer).getPath());
     }
+    return ((AbstractMultiFileWriter) writer).writers().stream().map(sfw -> sfw.getPath())
+      .collect(Collectors.toList());
   }
 
   /**
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DefaultCompactor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DefaultCompactor.java
index ad2384a..03e3a1b 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DefaultCompactor.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/compactions/DefaultCompactor.java
@@ -74,24 +74,22 @@ public class DefaultCompactor extends Compactor<StoreFileWriter> {
   @Override
   protected void abortWriter() throws IOException {
     abortWriter(writer);
+    // this step signals that the target file is no longer written and can be cleaned up
+    writer = null;
   }
 
-  protected void abortWriter(StoreFileWriter writer) throws IOException {
+  protected final void abortWriter(StoreFileWriter writer) throws IOException {
     Path leftoverFile = writer.getPath();
     try {
       writer.close();
     } catch (IOException e) {
       LOG.warn("Failed to close the writer after an unfinished compaction.", e);
-    } finally {
-      //this step signals that the target file is no longer writen and can be cleaned up
-      writer = null;
     }
     try {
       store.getFileSystem().delete(leftoverFile, false);
     } catch (IOException e) {
-      LOG.warn(
-        "Failed to delete the leftover file " + leftoverFile + " after an unfinished compaction.",
-        e);
+      LOG.warn("Failed to delete the leftover file {} after an unfinished compaction.",
+        leftoverFile, e);
     }
   }
 }