This is an automated email from the ASF dual-hosted git repository.
liuxiaocs7 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 9ffbae56d2a HBASE-30267 Backport HBASE-30266 Do not drop visibility
delete markers as redundant during minor compaction to branch-2 (#8448)
9ffbae56d2a is described below
commit 9ffbae56d2a382b2f6b80a59c1eb5b1ac71813d8
Author: Xiao Liu <[email protected]>
AuthorDate: Sun Jul 5 22:56:28 2026 +0800
HBASE-30267 Backport HBASE-30266 Do not drop visibility delete markers as
redundant during minor compaction to branch-2 (#8448)
Signed-off-by: Junegunn Choi <[email protected]>
(cherry picked from commit 3d93aa2fc36005a393cfad63e548248156765249)
---
.../visibility/VisibilityScanDeleteTracker.java | 14 +++
.../TestVisibilityLabelsWithDeletes.java | 94 ++++++++++++++
.../TestVisibilityScanDeleteTracker.java | 140 +++++++++++++++++++++
3 files changed, 248 insertions(+)
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java
index 180bd3cc4ea..3246ad8698c 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityScanDeleteTracker.java
@@ -328,6 +328,20 @@ public class VisibilityScanDeleteTracker extends
ScanDeleteTracker {
return DeleteResult.NOT_DELETED;
}
+ /**
+ * The inherited {@link ScanDeleteTracker#isRedundantDelete(ExtendedCell)}
is label-blind: it
+ * tracks deleteCell/deleteType/deleteTimestamp (and familyStamp) without
regard to the visibility
+ * tags of the delete markers. Reusing it during minor compaction could drop
a delete marker that
+ * only shadows differently-labeled data, resurrecting cells that should
stay deleted. So on
+ * cell-visibility tables we conservatively report no delete as redundant
(matching the
+ * {@link org.apache.hadoop.hbase.regionserver.querymatcher.DeleteTracker}
default), keeping every
+ * marker (tracked + included).
+ */
+ @Override
+ public boolean isRedundantDelete(ExtendedCell cell) {
+ return false;
+ }
+
@Override
public void reset() {
super.reset();
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java
index c1cd137cb99..1548b9874ce 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityLabelsWithDeletes.java
@@ -17,15 +17,18 @@
*/
package org.apache.hadoop.hbase.security.visibility;
+import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.IOException;
import java.io.InterruptedIOException;
import java.security.PrivilegedExceptionAction;
+import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.Random;
import org.apache.hadoop.hbase.Cell;
import org.apache.hadoop.hbase.CellScanner;
import org.apache.hadoop.hbase.CellUtil;
@@ -45,6 +48,7 @@ import org.apache.hadoop.hbase.client.Scan;
import org.apache.hadoop.hbase.client.Table;
import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
import
org.apache.hadoop.hbase.protobuf.generated.VisibilityLabelsProtos.VisibilityLabelsResponse;
+import org.apache.hadoop.hbase.regionserver.HRegion;
import org.apache.hadoop.hbase.testclassification.LargeTests;
import org.apache.hadoop.hbase.testclassification.SecurityTests;
import org.apache.hadoop.hbase.util.Bytes;
@@ -1734,6 +1738,96 @@ public class TestVisibilityLabelsWithDeletes extends
VisibilityLabelsWithDeletes
}
}
+ /**
+ * On a cell-visibility table, two DeleteColumn markers carrying different
labels shadow disjoint
+ * cells, so neither is redundant w.r.t. the other. A minor compaction must
not drop the
+ * lower-timestamp marker just because a higher-timestamp marker of a
different label was tracked
+ * first; doing so would resurrect data that must stay deleted. This is the
end-to-end regression
+ * guard for {@link VisibilityScanDeleteTracker#isRedundantDelete}.
+ * <p>
+ * The big put is larger than {@code hbase.hstore.compaction.max.size}, so
minor compaction
+ * excludes its file and merges only the two small delete-marker files
(isAllFiles=false, hence
+ * COMPACT_RETAIN_DELETES and MinorCompactionScanQueryMatcher, the path that
calls
+ * isRedundantDelete). The put's data stays in its own file, so dropping the
SECRET marker would
+ * make it visible again.
+ */
+ @Test
+ public void testDifferentLabelDeleteMarkersSurviveMinorCompaction(TestInfo
testInfo)
+ throws Exception {
+ setAuths();
+ final TableName tableName = TableName
+
.valueOf(TableNameTestExtension.cleanUpTestName(testInfo.getTestMethod().get().getName()));
+ // Minor compaction merges at most two files and skips any file larger
than 64KB.
+ ColumnFamilyDescriptorBuilder cfd =
ColumnFamilyDescriptorBuilder.newBuilder(fam)
+ .setMaxVersions(5).setConfiguration("hbase.hstore.compaction.min", "2")
+ .setConfiguration("hbase.hstore.compaction.max", "2")
+ .setConfiguration("hbase.hstore.compaction.max.size", "65536");
+ // Disable automatic compaction while the three HFiles are laid down, so
the minor compaction
+ // under test is exactly the one triggered explicitly below (no racing
background compaction).
+
TEST_UTIL.getAdmin().createTable(TableDescriptorBuilder.newBuilder(tableName)
+ .setCompactionEnabled(false).setColumnFamily(cfd.build()).build());
+
+ SUPERUSER.runAs((PrivilegedExceptionAction<Void>) () -> {
+ try (Connection connection = ConnectionFactory.createConnection(conf);
+ Table table = connection.getTable(tableName)) {
+ // Big SECRET-visible put at ts=40. Random bytes keep the file above
max.size so it is
+ // excluded from the minor compaction and retains the data the SECRET
delete shadows.
+ byte[] big = new byte[200 * 1024];
+ new Random(1).nextBytes(big);
+ Put put = new Put(row1);
+ put.addColumn(fam, qual, 40L, big);
+ put.setCellVisibility(new CellVisibility(SECRET));
+ table.put(put);
+ TEST_UTIL.getAdmin().flush(tableName);
+ // SECRET DeleteColumn at ts=50 shadows the SECRET put.
+ Delete dSecret = new Delete(row1);
+ dSecret.setCellVisibility(new CellVisibility(SECRET));
+ dSecret.addColumns(fam, qual, 50L);
+ table.delete(dSecret);
+ TEST_UTIL.getAdmin().flush(tableName);
+ // Newer CONFIDENTIAL DeleteColumn at ts=100 shadows disjoint
(CONFIDENTIAL) cells.
+ Delete dConf = new Delete(row1);
+ dConf.setCellVisibility(new CellVisibility(CONFIDENTIAL));
+ dConf.addColumns(fam, qual, 100L);
+ table.delete(dConf);
+ TEST_UTIL.getAdmin().flush(tableName);
+ }
+ return null;
+ });
+
+ // Before compaction the SECRET put is correctly hidden by the SECRET
delete.
+ assertEquals(0, countCells(tableName, SECRET));
+
+ // Synchronously minor-compact the two small delete files (the big put
file is excluded).
+ HRegion region = TEST_UTIL.getHBaseCluster().getRegions(tableName).get(0);
+ region.compact(false);
+ // Two files remain: the untouched big put file plus the single merged
delete file. This both
+ // confirms a minor compaction happened and that the big file was not
swept into a major one.
+ await().atMost(Duration.ofSeconds(60))
+ .untilAsserted(() -> assertEquals(2,
region.getStore(fam).getStorefilesCount()));
+
+ // The SECRET delete must still shadow the SECRET put: no resurrection.
+ assertEquals(0, countCells(tableName, SECRET));
+ }
+
+ private int countCells(TableName tableName, String... auths) throws
Exception {
+ return SUPERUSER.runAs((PrivilegedExceptionAction<Integer>) () -> {
+ try (Connection connection = ConnectionFactory.createConnection(conf);
+ Table table = connection.getTable(tableName)) {
+ Scan s = new Scan();
+ s.readVersions(5);
+ s.setAuthorizations(new Authorizations(auths));
+ try (ResultScanner scanner = table.getScanner(s)) {
+ int count = 0;
+ for (Result r : scanner) {
+ count += r.size();
+ }
+ return count;
+ }
+ }
+ });
+ }
+
@Test
public void
testDeleteFamilySpecificTimeStampWithMulipleVersionsDoneTwice(TestInfo testInfo)
throws Exception {
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityScanDeleteTracker.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityScanDeleteTracker.java
new file mode 100644
index 00000000000..bfd9848ca0d
--- /dev/null
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/visibility/TestVisibilityScanDeleteTracker.java
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.security.visibility;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.util.Collections;
+import java.util.List;
+import org.apache.hadoop.hbase.ArrayBackedTag;
+import org.apache.hadoop.hbase.CellComparatorImpl;
+import org.apache.hadoop.hbase.ExtendedCell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.Tag;
+import org.apache.hadoop.hbase.TagType;
+import org.apache.hadoop.hbase.regionserver.querymatcher.DeleteTracker;
+import org.apache.hadoop.hbase.testclassification.SecurityTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that {@link VisibilityScanDeleteTracker} never reports a delete
marker as redundant.
+ * <p>
+ * HBASE-30036 added {@link DeleteTracker#isRedundantDelete(ExtendedCell)} so
that minor compaction
+ * can drop a delete marker already covered by a previously tracked delete of
equal or broader
+ * scope. The base {@link
org.apache.hadoop.hbase.regionserver.querymatcher.ScanDeleteTracker}
+ * implements it purely from delete type / timestamp / qualifier, with no
regard to visibility
+ * labels.
+ * <p>
+ * On cell-visibility tables a delete marker only shadows cells whose
visibility expression matches
+ * (see {@link VisibilityScanDeleteTracker#isDeleted}). Two markers carrying
different labels cover
+ * disjoint cells, so neither is redundant w.r.t. the other. Reusing the
label-blind base logic
+ * would wrongly declare such a marker redundant; minor compaction would then
drop it and resurrect
+ * cells that must stay deleted. {@link VisibilityScanDeleteTracker} must
therefore conservatively
+ * report no delete as redundant.
+ */
[email protected](SecurityTests.TAG)
[email protected](SmallTests.TAG)
+public class TestVisibilityScanDeleteTracker {
+
+ private static final byte[] ROW = Bytes.toBytes("row");
+ private static final byte[] FAMILY = Bytes.toBytes("f");
+ private static final byte[] QUALIFIER = Bytes.toBytes("q");
+
+ // Placeholder visibility-tag payloads. The exact bytes are irrelevant here:
the tracker only
+ // needs the tag to be of type VISIBILITY_TAG_TYPE, and the two markers to
carry different ones.
+ private static final byte[] LABEL_A = new byte[] { 1 };
+ private static final byte[] LABEL_B = new byte[] { 2 };
+
+ /**
+ * A column-level delete must not be considered redundant just because a
same-qualifier
+ * DeleteColumn with a higher timestamp was already tracked: if they carry
different labels they
+ * shadow disjoint cells. The base tracker's {@code coveredByColumn} branch
would return true.
+ */
+ @Test
+ public void differentLabelColumnDeleteIsNotRedundant() {
+ VisibilityScanDeleteTracker tracker =
+ new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR);
+ // Tracked first (newest ts, as a compaction scan would present it).
+ tracker.add(deleteMarker(QUALIFIER, 100L, KeyValue.Type.DeleteColumn,
LABEL_A));
+
+ assertFalse(
+ tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L,
KeyValue.Type.DeleteColumn, LABEL_B)));
+ assertFalse(
+ tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L,
KeyValue.Type.Delete, LABEL_B)));
+ }
+
+ /**
+ * A family-level delete must not be considered redundant just because a
DeleteFamily with a
+ * higher timestamp set the family stamp: a label-less family delete only
shadows label-less
+ * cells, while a labeled one shadows that label's cells. The base tracker's
+ * {@code coveredByFamily} branch would return true.
+ */
+ @Test
+ public void differentLabelFamilyDeleteIsNotRedundant() {
+ VisibilityScanDeleteTracker tracker =
+ new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR);
+ // A label-less DeleteFamily is the only thing that advances familyStamp
(see add()).
+ tracker.add(deleteMarker(null, 100L, KeyValue.Type.DeleteFamily, null));
+
+ assertFalse(
+ tracker.isRedundantDelete(deleteMarker(null, 50L,
KeyValue.Type.DeleteFamily, LABEL_B)));
+ assertFalse(tracker
+ .isRedundantDelete(deleteMarker(null, 50L,
KeyValue.Type.DeleteFamilyVersion, LABEL_B)));
+ }
+
+ /**
+ * Even a same-label marker is kept: the base {@code coveredByColumn} branch
would (safely) treat
+ * a same-qualifier, same-label DeleteColumn of lower timestamp as
redundant, but the visibility
+ * tracker forgoes that optimization so the label-blind base logic can never
run. This pins the
+ * deliberately conservative contract against a future change that
re-enables it.
+ */
+ @Test
+ public void sameLabelColumnDeleteIsNotRedundant() {
+ VisibilityScanDeleteTracker tracker =
+ new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR);
+ tracker.add(deleteMarker(QUALIFIER, 100L, KeyValue.Type.DeleteColumn,
LABEL_A));
+
+ assertFalse(
+ tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L,
KeyValue.Type.DeleteColumn, LABEL_A)));
+ }
+
+ /**
+ * Even where the base answer would actually be correct (a label-less
DeleteColumn covered by a
+ * label-less one of higher timestamp), the tracker still reports no
redundancy, keeping the
+ * contract uniform across labeled and label-less markers.
+ */
+ @Test
+ public void labelLessColumnDeleteIsNotRedundant() {
+ VisibilityScanDeleteTracker tracker =
+ new VisibilityScanDeleteTracker(CellComparatorImpl.COMPARATOR);
+ tracker.add(deleteMarker(QUALIFIER, 100L, KeyValue.Type.DeleteColumn,
null));
+
+ assertFalse(
+ tracker.isRedundantDelete(deleteMarker(QUALIFIER, 50L,
KeyValue.Type.DeleteColumn, null)));
+ }
+
+ private static ExtendedCell deleteMarker(byte[] qualifier, long ts,
KeyValue.Type type,
+ byte[] label) {
+ List<Tag> tags = label == null
+ ? null
+ : Collections.singletonList(new
ArrayBackedTag(TagType.VISIBILITY_TAG_TYPE, label));
+ return new KeyValue(ROW, FAMILY, qualifier, ts, type, null, tags);
+ }
+}
