Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wchevreuil merged 2 commits into from
Jul 8, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1456fa
HBASE-21596 Delete for a specific cell version can bring back version…
wchevreuil Jul 1, 2020
9137fc7
Fixing previous failed tests and addressing review comments
wchevreuil Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 65 additions & 14 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3151,6 +3151,7 @@ public void prepareDeleteTimestamps(Mutation mutation, Map<byte[], List<Cell>> f
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++) {
Expand All @@ -3170,37 +3171,87 @@ public void prepareDeleteTimestamps(Mutation mutation, Map<byte[], List<Cell>> f
count = kvCount.get(qual);

Get get = new Get(CellUtil.cloneRow(cell));
get.readVersions(count);
get.addColumn(family, qual);
get.readVersions(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);
}
Comment on lines 3175 to 3187
Copy link
Contributor

@virajjasani virajjasani Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer using a boolean to make single call to updateDeleteLatestVersionTimestamp?

          boolean updateDelTs=false;
          if (coprocessorHost != null) {
            if (!coprocessorHost.prePrepareTimeStampForDeleteVersion(mutation, cell,
                byteNow, get)) {
              updateDelTs=true;
            }
          } else {
            updateDelTs=true;
          }
          if(updateDelTs){
            updateDeleteLatestVersionTimestamp(cell, get, count,
              this.htableDescriptor.getColumnFamily(family).getMaxVersions(),
              byteNow, deleteCells);
          }

Only if you feel this is more readable :)

} else {
PrivateCellUtil.updateLatestStamp(cell, byteNow);
deleteCells.add(cell);
}
}
e.setValue(deleteCells);
}
}

void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, byte[] byteNow)
throws IOException {
List<Cell> result = get(get, false);

private void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, int maxVersions,
byte[] byteNow, List<Cell> deleteCells) throws IOException {
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) {
Copy link
Contributor

@virajjasani virajjasani Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result sorting doesn't seem useful for this condition. Can we avoid sorting for this?

if (result.size() < count) {
..
..
  deleteCells.addAll(cells);
  return;
}

result.sort();

if (result.size() > count){
..
..
}else{
..
}
deleteCells.addAll(cells);

Copy link
Contributor Author

@wchevreuil wchevreuil Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

// 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;
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;
}
}
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);
Copy link
Contributor

@virajjasani virajjasani Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if sorting of result is required here. If not required, rest looks good.

Copy link
Contributor Author

@wchevreuil wchevreuil Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed, because we don't have to worry about additional versions, we only need to put a single marker for current TS.

PrivateCellUtil.setTimestamp(cell, getCell.getTimestamp());
cells.add(cell);
}
Cell getCell = result.get(count - 1);
PrivateCellUtil.setTimestamp(cell, getCell.getTimestamp());
deleteCells.addAll(cells);
}

@Override
Expand Down
10 changes: 6 additions & 4 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/TimestampTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,22 @@ public static void doTestDelete(final Table table, FlushCache flusher)
// 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.
Expand Down
16 changes: 8 additions & 8 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1741,15 +1741,15 @@ public void testDeletes() throws Exception {
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] },
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);

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] },
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);
Expand All @@ -1776,17 +1776,17 @@ public void testDeletes() throws Exception {
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] },
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);

// The Scanner returns the previous values, the expected-naive-unexpected behavior

scan = new Scan().withStartRow(ROW);
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] },
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

Expand Down
8 changes: 4 additions & 4 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide5.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1856,8 +1856,8 @@ public void testDeletesWithReverseScan() throws Exception {
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]}, 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);
Expand Down Expand Up @@ -1885,8 +1885,8 @@ public void testDeletesWithReverseScan() throws Exception {
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]}, 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
Expand Down
67 changes: 67 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeepDeletes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,6 +941,73 @@ public void testWithTTL() throws Exception {
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);
Expand Down
11 changes: 5 additions & 6 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinorCompaction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,12 @@ public void testMinorCompactionWithDeleteColumn1() throws Exception {
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
Expand Down