HBASE-21596 Delete for a specific cell version can bring back version… (#2009)

Signed-off-by: Viraj Jasani <vjasani@apache.org>

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

@@ -3151,6 +3151,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
       List<Cell> cells = e.getValue();
       assert cells instanceof RandomAccess;
 
+      List<Cell> deleteCells = new ArrayList<>();
       Map<byte[], Integer> kvCount = new TreeMap<>(Bytes.BYTES_COMPARATOR);
       int listSize = cells.size();
       for (int i=0; i < listSize; i++) {
@@ -3170,37 +3171,87 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
           count = kvCount.get(qual);
 
           Get get = new Get(CellUtil.cloneRow(cell));
-          get.readVersions(count);
+          get.readVersions(Integer.MAX_VALUE);
-          get.addColumn(family, qual);
           if (coprocessorHost != null) {
             if (!coprocessorHost.prePrepareTimeStampForDeleteVersion(mutation, cell,
                 byteNow, get)) {
-              updateDeleteLatestVersionTimestamp(cell, get, count, byteNow);
+              updateDeleteLatestVersionTimestamp(cell, get, count,
+                  this.htableDescriptor.getColumnFamily(family).getMaxVersions(),
+                    byteNow, deleteCells);
+
             }
           } else {
-            updateDeleteLatestVersionTimestamp(cell, get, count, byteNow);
+            updateDeleteLatestVersionTimestamp(cell, get, count,
+                this.htableDescriptor.getColumnFamily(family).getMaxVersions(),
+                  byteNow, deleteCells);
           }
         } else {
           PrivateCellUtil.updateLatestStamp(cell, byteNow);
+          deleteCells.add(cell);
         }
       }
+      e.setValue(deleteCells);
     }
   }
 
-  void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, byte[] byteNow)
+  private void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, int maxVersions,
-      throws IOException {
+      byte[] byteNow, List<Cell> deleteCells) throws IOException {
-    List<Cell> result = get(get, false);
+    List<Cell> result = new ArrayList<>(deleteCells);
-
+    Scan scan = new Scan(get);
+    scan.setRaw(true);
+    this.getScanner(scan).next(result);
+    List<Cell> cells = new ArrayList<>();
     if (result.size() < count) {
       // Nothing to delete
       PrivateCellUtil.updateLatestStamp(cell, byteNow);
-      return;
+      cells.add(cell);
+      deleteCells.addAll(cells);
+    } else if (result.size() > count) {
+      int currentVersion = 0;
+      long latestCellTS = Long.MAX_VALUE;
+      result.sort((cell1, cell2) -> {
+        if(cell1.getTimestamp()>cell2.getTimestamp()){
+          return -1;
+        } else if(cell1.getTimestamp()<cell2.getTimestamp()){
+          return 1;
+        } else {
+          if(CellUtil.isDelete(cell1)){
+            return -1;
+          } else if (CellUtil.isDelete(cell2)){
+            return 1;
           }
-    if (result.size() > count) {
-      throw new RuntimeException("Unexpected size: " + result.size());
         }
-    Cell getCell = result.get(count - 1);
+        return 0;
+      });
+      for(Cell getCell : result){
+        if(!(CellUtil.matchingFamily(getCell, cell) && CellUtil.matchingQualifier(getCell, cell))){
+          continue;
+        }
+        if(!PrivateCellUtil.isDeleteType(getCell) && getCell.getTimestamp()!=latestCellTS){
+          if (currentVersion >= maxVersions) {
+            Cell tempCell = null;
+            try {
+              tempCell = PrivateCellUtil.deepClone(cell);
+            } catch (CloneNotSupportedException e) {
+              throw new IOException(e);
+            }
+            PrivateCellUtil.setTimestamp(tempCell, getCell.getTimestamp());
+            cells.add(tempCell);
+          } else if (currentVersion == 0) {
             PrivateCellUtil.setTimestamp(cell, getCell.getTimestamp());
+            cells.add(cell);
+          }
+          currentVersion++;
+        }
+        latestCellTS = getCell.getTimestamp();
+      }
+
+    } else {
+      Cell getCell = result.get(0);
+      PrivateCellUtil.setTimestamp(cell, getCell.getTimestamp());
+      cells.add(cell);
+    }
+    deleteCells.addAll(cells);
   }
 
   @Override

hbase-server/src/test/java/org/apache/hadoop/hbase/TimestampTestBase.java

@@ -70,20 +70,22 @@ public class TimestampTestBase {
     // If I delete w/o specifying a timestamp, this means I'm deleting the latest.
     delete(table);
     // Verify that I get back T2 through T1 -- that the latest version has been deleted.
-    assertVersions(table, new long [] {T2, T1, T0});
+    // Since there were originally 4 puts before the delete, T0 is gone now,
+    // so after deleting latest, there should be only T2 and T1
+    assertVersions(table, new long [] {T2, T1});
 
     // Flush everything out to disk and then retry
     flusher.flushcache();
-    assertVersions(table, new long [] {T2, T1, T0});
+    assertVersions(table, new long [] {T2, T1});
 
     // Now add, back a latest so I can test remove other than the latest.
     put(table);
     assertVersions(table, new long [] {HConstants.LATEST_TIMESTAMP, T2, T1});
     delete(table, T2);
-    assertVersions(table, new long [] {HConstants.LATEST_TIMESTAMP, T1, T0});
+    assertVersions(table, new long [] {HConstants.LATEST_TIMESTAMP, T1});
     // Flush everything out to disk and then retry
     flusher.flushcache();
-    assertVersions(table, new long [] {HConstants.LATEST_TIMESTAMP, T1, T0});
+    assertVersions(table, new long [] {HConstants.LATEST_TIMESTAMP, T1});
 
     // Now try deleting all from T2 back inclusive (We first need to add T2
     // back into the mix and to make things a little interesting, delete and then readd T1.

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java

@@ -1741,15 +1741,15 @@ public class TestFromClientSide extends FromClientSideBase {
       get.addColumn(FAMILIES[0], QUALIFIER);
       get.readVersions(Integer.MAX_VALUE);
       result = ht.get(get);
-      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[1], ts[2], ts[3] },
+      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] {  ts[2], ts[3] },
-        new byte[][] { VALUES[1], VALUES[2], VALUES[3] }, 0, 2);
+        new byte[][] {  VALUES[2], VALUES[3] }, 0, 1);
 
       scan = new Scan().withStartRow(ROW);
       scan.addColumn(FAMILIES[0], QUALIFIER);
       scan.readVersions(Integer.MAX_VALUE);
       result = getSingleScanResult(ht, scan);
-      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[1], ts[2], ts[3] },
+      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] {  ts[2], ts[3] },
-        new byte[][] { VALUES[1], VALUES[2], VALUES[3] }, 0, 2);
+        new byte[][] {  VALUES[2], VALUES[3] }, 0, 1);
 
       // Test for HBASE-1847
       delete = new Delete(ROW);
@@ -1776,8 +1776,8 @@ public class TestFromClientSide extends FromClientSideBase {
       get.addFamily(FAMILIES[0]);
       get.readVersions(Integer.MAX_VALUE);
       result = ht.get(get);
-      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[1], ts[2], ts[3] },
+      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] {  ts[2], ts[3] },
-        new byte[][] { VALUES[1], VALUES[2], VALUES[3] }, 0, 2);
+        new byte[][] {  VALUES[2], VALUES[3] }, 0, 1);
 
       // The Scanner returns the previous values, the expected-naive-unexpected behavior
 
@@ -1785,8 +1785,8 @@ public class TestFromClientSide extends FromClientSideBase {
       scan.addFamily(FAMILIES[0]);
       scan.readVersions(Integer.MAX_VALUE);
       result = getSingleScanResult(ht, scan);
-      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[1], ts[2], ts[3] },
+      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] {  ts[2], ts[3] },
-        new byte[][] { VALUES[1], VALUES[2], VALUES[3] }, 0, 2);
+        new byte[][] {  VALUES[2], VALUES[3] }, 0, 1);
 
       // Test deleting an entire family from one row but not the other various ways
 

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide5.java

@@ -1856,8 +1856,8 @@ public class TestFromClientSide5 extends FromClientSideBase {
       scan.addColumn(FAMILIES[0], QUALIFIER);
       scan.readVersions(Integer.MAX_VALUE);
       result = getSingleScanResult(ht, scan);
-      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[]{ts[1],
+      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[]{ ts[2], ts[3]},
-              ts[2], ts[3]}, new byte[][]{VALUES[1], VALUES[2], VALUES[3]}, 0, 2);
+        new byte[][]{ VALUES[2], VALUES[3]}, 0, 1);
 
       // Test for HBASE-1847
       delete = new Delete(ROW);
@@ -1885,8 +1885,8 @@ public class TestFromClientSide5 extends FromClientSideBase {
       scan.addFamily(FAMILIES[0]);
       scan.readVersions(Integer.MAX_VALUE);
       result = getSingleScanResult(ht, scan);
-      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[]{ts[1],
+      assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[]{ ts[2], ts[3]},
-              ts[2], ts[3]}, new byte[][]{VALUES[1], VALUES[2], VALUES[3]}, 0, 2);
+        new byte[][]{VALUES[2], VALUES[3]}, 0, 1);
 
       // Test deleting an entire family from one row but not the other various
       // ways

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

@@ -941,6 +941,73 @@ public class TestKeepDeletes {
     HBaseTestingUtility.closeRegionAndWAL(region);
   }
 
+  /**
+   * No more than max versions should be retained. For a CF with max versions 1,
+   * scans/get should not yield any results after a delete.
+   *
+   * @throws IOException if test faces any issues while creating/cleaning up necessary region.
+   */
+  @Test
+  public void testDeleteConsistentWithOneVersion() throws IOException {
+    HTableDescriptor htd = hbu.createTableDescriptor(TableName.valueOf(name.getMethodName()), 0, 1,
+        HConstants.FOREVER, KeepDeletedCells.TRUE);
+    HRegion region = hbu.createLocalHRegion(htd, null, null);
+
+    long ts = EnvironmentEdgeManager.currentTime();
+    Put p = new Put(T1, ts);
+    p.addColumn(c0, c0, T1);
+    region.put(p);
+
+    p = new Put(T1, ts+1);
+    p.addColumn(c0, c0, T2);
+    region.put(p);
+
+    checkGet(region, T1, c0, c0, ts+2, T2);
+
+    Delete delete = new Delete(T1);
+    delete.addColumn(c0, c0, HConstants.LATEST_TIMESTAMP);
+    region.delete(delete);
+
+    Get get = new Get(T1);
+    Result result = region.get(get);
+
+    assertTrue("Get should not return any results.", result.isEmpty());
+
+    HBaseTestingUtility.closeRegionAndWAL(region);
+  }
+
+  /**
+   * No more than max versions should be retained. For a CF with max versions 2,
+   * scans/get should yield second version after delete.
+   *
+   * @throws IOException if test faces any issues while creating/cleaning up necessary region.
+   */
+  @Test
+  public void testDeleteConsistentWithTwoVersions() throws Exception {
+    HTableDescriptor htd = hbu.createTableDescriptor(TableName.valueOf(name.getMethodName()), 0, 2,
+        HConstants.FOREVER, KeepDeletedCells.TRUE);
+    HRegion region = hbu.createLocalHRegion(htd, null, null);
+
+    long ts = EnvironmentEdgeManager.currentTime();
+    Put p = new Put(T1, ts);
+    p.addColumn(c0, c0, T1);
+    region.put(p);
+
+    p = new Put(T1, ts+1);
+    p.addColumn(c0, c0, T2);
+    region.put(p);
+
+    checkGet(region, T1, c0, c0, ts+2, T2, T1);
+
+    Delete delete = new Delete(T1);
+    delete.addColumn(c0, c0, HConstants.LATEST_TIMESTAMP);
+    region.delete(delete);
+
+    checkGet(region, T1, c0, c0, ts+2, T1);
+
+    HBaseTestingUtility.closeRegionAndWAL(region);
+  }
+
   private void checkGet(Region region, byte[] row, byte[] fam, byte[] col,
       long time, byte[]... vals) throws IOException {
     Get g = new Get(row);

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

@@ -123,13 +123,12 @@ public class TestMinorCompaction {
   public void testMinorCompactionWithDeleteColumn2() throws Exception {
     Delete dc = new Delete(secondRowBytes);
     dc.addColumn(fam2, col2);
-    /* compactionThreshold is 3. The table has 4 versions: 0, 1, 2, and 3.
+    /* compactionThreshold is 3. We had inserte a total of 4 versions (0, 1, 2, and 3),
-     * we only delete the latest version. One might expect to see only
+     * but family is configured for 3 versions only, thus first one (0) was already overridden
-     * versions 1 and 2. HBase differs, and gives us 0, 1 and 2.
+     * when we added the fourth (3). Then after deleting the most recent (3), there should be only
-     * This is okay as well. Since there was no compaction done before the
+     * 2 versions (1, 2).
-     * delete, version 0 seems to stay on.
      */
-    testMinorCompactionWithDelete(dc, 3);
+    testMinorCompactionWithDelete(dc, 2);
   }
 
   @Test