From 8de628730bc2b1978aa69d31e1e7943b5fb2fa8b Mon Sep 17 00:00:00 2001 From: Wellington Chevreuil Date: Mon, 24 Dec 2018 14:46:55 +0000 Subject: [PATCH] HBASE-21596 - HBase Shell "delete" command can cause older versions to be shown even if VERSIONS is configured as 1 --- .../hadoop/hbase/regionserver/HRegion.java | 80 +++++++++++++++---- .../hadoop/hbase/TimestampTestBase.java | 10 ++- .../hbase/client/TestFromClientSide.java | 32 ++++---- .../hbase/regionserver/TestKeepDeletes.java | 67 ++++++++++++++++ .../regionserver/TestMinorCompaction.java | 11 ++- 5 files changed, 160 insertions(+), 40 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 4b6da53edd..1be601bbf4 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -3000,6 +3000,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi List cells = e.getValue(); assert cells instanceof RandomAccess; + List deleteCells = new ArrayList<>(); Map kvCount = new TreeMap<>(Bytes.BYTES_COMPARATOR); int listSize = cells.size(); for (int i=0; i < listSize; i++) { @@ -3019,37 +3020,88 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi count = kvCount.get(qual); Get get = new Get(CellUtil.cloneRow(cell)); - get.setMaxVersions(count); - get.addColumn(family, qual); + get.setMaxVersions(Integer.MAX_VALUE); 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) - throws IOException { - List result = get(get, false); - + void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, int maxVersions, + byte[] byteNow, List deleteCells) throws IOException { + List result = new ArrayList<>(); + result.addAll(deleteCells); + Scan scan = new Scan(get); + scan.setRaw(true); + this.getScanner(scan).next(result); + result.sort((cell1, cell2) -> { + if(cell1.getTimestamp()>cell2.getTimestamp()){ + return -1; + } else if(cell1.getTimestamp() cells = new ArrayList<>(); if (result.size() < count) { // Nothing to delete PrivateCellUtil.updateLatestStamp(cell, byteNow); - return; - } - if (result.size() > count) { - throw new RuntimeException("Unexpected size: " + result.size()); + cells.add(cell); + deleteCells.addAll(cells); + } else if (result.size() > count) { + int currentVersion = 0; + long latestCellTS = Long.MAX_VALUE; + 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); } - Cell getCell = result.get(count - 1); - PrivateCellUtil.setTimestamp(cell, getCell.getTimestamp()); + deleteCells.addAll(cells); } @Override diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TimestampTestBase.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TimestampTestBase.java index 8e8d4ef083..96cfe03bf6 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TimestampTestBase.java +++ b/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. diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java index 65bc3f619b..c8485b7576 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java @@ -1965,18 +1965,18 @@ public class TestFromClientSide { get.setMaxVersions(Integer.MAX_VALUE); result = ht.get(get); assertNResult(result, ROW, FAMILIES[0], QUALIFIER, - new long [] {ts[1], ts[2], ts[3]}, - new byte[][] {VALUES[1], VALUES[2], VALUES[3]}, - 0, 2); + new long [] { ts[2], ts[3]}, + new byte[][] { VALUES[2], VALUES[3]}, + 0, 1); scan = new Scan(ROW); scan.addColumn(FAMILIES[0], QUALIFIER); scan.setMaxVersions(Integer.MAX_VALUE); result = getSingleScanResult(ht, scan); assertNResult(result, ROW, FAMILIES[0], QUALIFIER, - new long [] {ts[1], ts[2], ts[3]}, - new byte[][] {VALUES[1], VALUES[2], VALUES[3]}, - 0, 2); + new long [] {ts[2], ts[3]}, + new byte[][] {VALUES[2], VALUES[3]}, + 0, 1); // Test for HBASE-1847 delete = new Delete(ROW); @@ -2005,9 +2005,9 @@ public class TestFromClientSide { get.setMaxVersions(Integer.MAX_VALUE); result = ht.get(get); assertNResult(result, ROW, FAMILIES[0], QUALIFIER, - new long [] {ts[1], ts[2], ts[3]}, - new byte[][] {VALUES[1], VALUES[2], VALUES[3]}, - 0, 2); + new long [] {ts[2], ts[3]}, + new byte[][] {VALUES[2], VALUES[3]}, + 0, 1); // The Scanner returns the previous values, the expected-naive-unexpected behavior @@ -2016,9 +2016,9 @@ public class TestFromClientSide { scan.setMaxVersions(Integer.MAX_VALUE); result = getSingleScanResult(ht, scan); assertNResult(result, ROW, FAMILIES[0], QUALIFIER, - new long [] {ts[1], ts[2], ts[3]}, - new byte[][] {VALUES[1], VALUES[2], VALUES[3]}, - 0, 2); + new long [] {ts[2], ts[3]}, + new byte[][] {VALUES[2], VALUES[3]}, + 0, 1); // Test deleting an entire family from one row but not the other various ways @@ -5989,8 +5989,8 @@ public class TestFromClientSide { scan.addColumn(FAMILIES[0], QUALIFIER); scan.setMaxVersions(Integer.MAX_VALUE); result = getSingleScanResult(ht, scan); - assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[1], - ts[2], ts[3] }, new byte[][] { VALUES[1], VALUES[2], VALUES[3] }, 0, 2); + assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[2], ts[3] }, + new byte[][] { VALUES[2], VALUES[3] }, 0, 1); // Test for HBASE-1847 delete = new Delete(ROW); @@ -6018,8 +6018,8 @@ public class TestFromClientSide { scan.addFamily(FAMILIES[0]); scan.setMaxVersions(Integer.MAX_VALUE); result = getSingleScanResult(ht, scan); - assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[1], - ts[2], ts[3] }, new byte[][] { VALUES[1], VALUES[2], VALUES[3] }, 0, 2); + assertNResult(result, ROW, FAMILIES[0], QUALIFIER, new long[] { ts[2], ts[3] }, + new byte[][] { VALUES[2], VALUES[3] }, 0, 1); // Test deleting an entire family from one row but not the other various // ways diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java index 12a53124f7..1441ce7898 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java @@ -942,6 +942,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(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(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); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinorCompaction.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinorCompaction.java index e53ed363e5..6c14c7a372 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinorCompaction.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinorCompaction.java @@ -119,13 +119,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. - * we only delete the latest version. One might expect to see only - * versions 1 and 2. HBase differs, and gives us 0, 1 and 2. - * This is okay as well. Since there was no compaction done before the - * delete, version 0 seems to stay on. + /* compactionThreshold is 3. We had inserte a total of 4 versions (0, 1, 2, and 3), + * but family is configured for 3 versions only, thus first one (0) was already overridden + * when we added the fourth (3). Then after deleting the most recent (3), there should be only + * 2 versions (1, 2). */ - testMinorCompactionWithDelete(dc, 3); + testMinorCompactionWithDelete(dc, 2); } @Test -- 2.17.2 (Apple Git-113)