HBASE-25720 Sync WAL stuck when prepare flush cache will prevent flush cache and cause OOM

Author: sunhelly

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -2842,7 +2842,13 @@ protected PrepareFlushResult internalPrepareFlushCache(WAL wal, long myseqid,
     String s = "Finished memstore snapshotting " + this + ", syncing WAL and waiting on mvcc, " +
         "flushsize=" + totalSizeOfFlushableStores;
     status.setStatus(s);
-    doSyncOfUnflushedWALChanges(wal, getRegionInfo());
+
+    try {
+      doSyncOfUnflushedWALChanges(wal, getRegionInfo());
+    } catch (Throwable t) {
+      status.abort("Sync unflushed WAL changes failed: " + StringUtils.stringifyException(t));
+      fatalForFlushCache(t);
+    }
     return new PrepareFlushResult(storeFlushCtxs, committedFiles, storeFlushableSize, startTime,
         flushOpSeqId, flushedSeqId, totalSizeOfFlushableStores);
   }
@@ -3032,22 +3038,8 @@ FlushResultImpl internalFlushCacheAndCommit(WAL wal, MonitoredTask status,
         }
         wal.abortCacheFlush(this.getRegionInfo().getEncodedNameAsBytes());
       }
-      DroppedSnapshotException dse = new DroppedSnapshotException("region: " +
-        Bytes.toStringBinary(getRegionInfo().getRegionName()), t);
       status.abort("Flush failed: " + StringUtils.stringifyException(t));
-
-      // Callers for flushcache() should catch DroppedSnapshotException and abort the region server.
-      // However, since we may have the region read lock, we cannot call close(true) here since
-      // we cannot promote to a write lock. Instead we are setting closing so that all other region
-      // operations except for close will be rejected.
-      this.closing.set(true);
-
-      if (rsServices != null) {
-        // This is a safeguard against the case where the caller fails to explicitly handle aborting
-        rsServices.abort("Replay of WAL required. Forcing server shutdown", dse);
-      }
-
-      throw dse;
+      fatalForFlushCache(t);
     }
 
     // If we get to here, the HStores have been written.
@@ -3093,6 +3085,24 @@ FlushResultImpl internalFlushCacheAndCommit(WAL wal, MonitoredTask status,
           FlushResult.Result.FLUSHED_NO_COMPACTION_NEEDED, flushOpSeqId);
   }
 
+  private void fatalForFlushCache(Throwable t) throws DroppedSnapshotException {
+    DroppedSnapshotException dse = new DroppedSnapshotException("region: " +
+      Bytes.toStringBinary(getRegionInfo().getRegionName()), t);
+
+    // Callers for flushcache() should catch DroppedSnapshotException and abort the region server.
+    // However, since we may have the region read lock, we cannot call close(true) here since
+    // we cannot promote to a write lock. Instead we are setting closing so that all other region
+    // operations except for close will be rejected.
+    this.closing.set(true);
+
+    if (rsServices != null) {
+      // This is a safeguard against the case where the caller fails to explicitly handle aborting
+      rsServices.abort("Replay of WAL required. Forcing server shutdown", dse);
+    }
+
+    throw dse;
+  }
+
   /**
    * Method to safely get the next sequence number.
    * @return Next sequence number unassociated with any actual edit.

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java

@@ -1276,9 +1276,6 @@ public long getSyncedLength() {
     try {
       region.flush(true);
       fail("This should have thrown exception");
-    } catch (DroppedSnapshotException unexpected) {
-      // this should not be a dropped snapshot exception. Meaning that RS will not abort
-      throw unexpected;
     } catch (IOException expected) {
       // expected
     }

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java

@@ -535,20 +535,6 @@ public void postLogRoll(Path oldFile, Path newFile) {
       assertTrue(loggedRows.contains("row1004"));
       assertTrue(loggedRows.contains("row1005"));
 
-      // flush all regions
-      for (HRegion r : server.getOnlineRegionsLocalContext()) {
-        try {
-          r.flush(true);
-        } catch (Exception e) {
-          // This try/catch was added by HBASE-14317. It is needed
-          // because this issue tightened up the semantic such that
-          // a failed append could not be followed by a successful
-          // sync. What is coming out here is a failed sync, a sync
-          // that used to 'pass'.
-          LOG.info(e.toString(), e);
-        }
-      }
-
       ResultScanner scanner = table.getScanner(new Scan());
       try {
         for (int i = 2; i <= 5; i++) {