Refactor bloom filter handling in AbstractMultiTenantReader and StoreFileReader to improve validation. Introduce checks for section count in bloom filter retrieval methods. Update passesDeleteFamilyBloomFilter and passesGeneralRowBloomFilter methods to streamline logic and enhance error handling. Add a new test for multi-tenant bloom filter correctness across multiple sections.

Author: sairampola

hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/AbstractMultiTenantReader.java

@@ -761,12 +761,18 @@ public long getDeleteFamilyBloomCount() throws IOException {
 
   @Override
   public BloomFilter getGeneralBloomFilterInstance() throws IOException {
+    if (getSectionCount() != 1) {
+      return null;
+    }
     SectionBloomState state = findSectionBloomState(true, false);
     return state != null ? state.getGeneralBloom() : null;
   }
 
   @Override
   public BloomFilter getDeleteFamilyBloomFilterInstance() throws IOException {
+    if (getSectionCount() != 1) {
+      return null;
+    }
     SectionBloomState state = findSectionBloomState(false, true);
     return state != null ? state.getDeleteBloom() : null;
   }

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

@@ -300,38 +300,33 @@ boolean passesBloomFilter(Scan scan, final SortedSet<byte[]> columns) {
   }
 
   public boolean passesDeleteFamilyBloomFilter(byte[] row, int rowOffset, int rowLen) {
-    if (reader instanceof MultiTenantBloomSupport) {
-      try {
-        return ((MultiTenantBloomSupport) reader).passesDeleteFamilyBloomFilter(row, rowOffset,
-          rowLen);
-      } catch (IOException e) {
-        LOG.warn("Failed multi-tenant delete-family bloom check, proceeding without", e);
-        return true;
-      }
-    }
-    // Cache Bloom filter as a local variable in case it is set to null by
-    // another thread on an IO error.
     BloomFilter bloomFilter = this.deleteFamilyBloomFilter;
 
-    // Empty file or there is no delete family at all
     if (reader.getTrailer().getEntryCount() == 0 || deleteFamilyCnt == 0) {
       return false;
     }
 
-    if (bloomFilter == null) {
+    if (bloomFilter != null) {
+      try {
+        if (!bloomFilter.supportsAutoLoading()) {
+          return true;
+        }
+        return bloomFilter.contains(row, rowOffset, rowLen, null);
+      } catch (IllegalArgumentException e) {
+        LOG.error("Bad Delete Family bloom filter data -- proceeding without", e);
+        setDeleteFamilyBloomFilterFaulty();
+      }
       return true;
     }
-
-    try {
-      if (!bloomFilter.supportsAutoLoading()) {
+    if (reader instanceof MultiTenantBloomSupport) {
+      try {
+        return ((MultiTenantBloomSupport) reader).passesDeleteFamilyBloomFilter(row, rowOffset,
+          rowLen);
+      } catch (IOException e) {
+        LOG.warn("Failed multi-tenant delete-family bloom check, proceeding without", e);
         return true;
       }
-      return bloomFilter.contains(row, rowOffset, rowLen, null);
-    } catch (IllegalArgumentException e) {
-      LOG.error("Bad Delete Family bloom filter data -- proceeding without", e);
-      setDeleteFamilyBloomFilterFaulty();
     }
-
     return true;
   }
 
@@ -341,6 +336,13 @@ public boolean passesDeleteFamilyBloomFilter(byte[] row, int rowOffset, int rowL
    * @return True if passes
    */
   private boolean passesGeneralRowBloomFilter(byte[] row, int rowOffset, int rowLen) {
+    BloomFilter bloomFilter = this.generalBloomFilter;
+    if (bloomFilter != null) {
+      if (rowOffset != 0 || rowLen != row.length) {
+        throw new AssertionError("For row-only Bloom filters the row must occupy the whole array");
+      }
+      return checkGeneralBloomFilter(row, null, bloomFilter);
+    }
     if (reader instanceof MultiTenantBloomSupport) {
       try {
         boolean passed =
@@ -352,19 +354,8 @@ private boolean passesGeneralRowBloomFilter(byte[] row, int rowOffset, int rowLe
         return true;
       }
     }
-    BloomFilter bloomFilter = this.generalBloomFilter;
-    if (bloomFilter == null) {
-      incrementBloomEligible();
-      return true;
-    }
-
-    // Used in ROW bloom
-    byte[] key = null;
-    if (rowOffset != 0 || rowLen != row.length) {
-      throw new AssertionError("For row-only Bloom filters the row must occupy the whole array");
-    }
-    key = row;
-    return checkGeneralBloomFilter(key, null, bloomFilter);
+    incrementBloomEligible();
+    return true;
   }
 
   /**
@@ -373,6 +364,16 @@ private boolean passesGeneralRowBloomFilter(byte[] row, int rowOffset, int rowLe
    * @return True if passes
    */
   public boolean passesGeneralRowColBloomFilter(ExtendedCell cell) {
+    BloomFilter bloomFilter = this.generalBloomFilter;
+    if (bloomFilter != null) {
+      ExtendedCell kvKey;
+      if (cell.getTypeByte() == KeyValue.Type.Maximum.getCode() && cell.getFamilyLength() == 0) {
+        kvKey = cell;
+      } else {
+        kvKey = PrivateCellUtil.createFirstOnRowCol(cell);
+      }
+      return checkGeneralBloomFilter(null, kvKey, bloomFilter);
+    }
     if (reader instanceof MultiTenantBloomSupport) {
       try {
         boolean passed = ((MultiTenantBloomSupport) reader).passesGeneralRowColBloomFilter(cell);
@@ -383,20 +384,8 @@ public boolean passesGeneralRowColBloomFilter(ExtendedCell cell) {
         return true;
       }
     }
-    BloomFilter bloomFilter = this.generalBloomFilter;
-    if (bloomFilter == null) {
-      incrementBloomEligible();
-      return true;
-    }
-    // Used in ROW_COL bloom
-    ExtendedCell kvKey = null;
-    // Already if the incoming key is a fake rowcol key then use it as it is
-    if (cell.getTypeByte() == KeyValue.Type.Maximum.getCode() && cell.getFamilyLength() == 0) {
-      kvKey = cell;
-    } else {
-      kvKey = PrivateCellUtil.createFirstOnRowCol(cell);
-    }
-    return checkGeneralBloomFilter(null, kvKey, bloomFilter);
+    incrementBloomEligible();
+    return true;
   }
 
   /**
@@ -418,6 +407,10 @@ private boolean passesGeneralRowPrefixBloomFilter(Scan scan) {
       rowPrefix = Bytes.copy(row, 0, prefixLength);
     }
 
+    BloomFilter bloomFilter = this.generalBloomFilter;
+    if (bloomFilter != null) {
+      return checkGeneralBloomFilter(rowPrefix, null, bloomFilter);
+    }
     if (reader instanceof MultiTenantBloomSupport) {
       try {
         boolean passed = ((MultiTenantBloomSupport) reader)
@@ -429,13 +422,8 @@ private boolean passesGeneralRowPrefixBloomFilter(Scan scan) {
         return true;
       }
     }
-
-    BloomFilter bloomFilter = this.generalBloomFilter;
-    if (bloomFilter == null) {
-      incrementBloomEligible();
-      return true;
-    }
-    return checkGeneralBloomFilter(rowPrefix, null, bloomFilter);
+    incrementBloomEligible();
+    return true;
   }
 
   private boolean checkGeneralBloomFilter(byte[] key, Cell kvKey, BloomFilter bloomFilter) {

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

@@ -118,6 +118,7 @@ public class StoreFileWriter implements CellSink, ShipperListener {
   private int livePutCellCount;
   private final int maxVersions;
   private final boolean newVersionBehavior;
+  private volatile boolean closed = false;
 
   /**
    * Creates an HFile.Writer that also write helpful meta data.
@@ -312,6 +313,10 @@ public BloomFilterWriter getGeneralBloomWriter() {
   }
 
   public void close() throws IOException {
+    if (closed) {
+      return;
+    }
+    closed = true;
     liveFileWriter.appendFileInfo(HISTORICAL_KEY, Bytes.toBytes(false));
     liveFileWriter.close();
     if (historicalFileWriter != null) {