HBASE-21596 Delete for a specific cell version can bring back version…#2009
HBASE-21596 Delete for a specific cell version can bring back version…#2009wchevreuil merged 2 commits intoapache:masterfrom
Conversation
…s above VERSIONS limit
wchevreuil
requested review from
bharathv,
joshelser,
meszibalu,
saintstack and
virajjasani
|
🎊 +1 overall
This message was automatically generated. |
| throws IOException { | ||
| List<Cell> result = get(get, false); | ||
|
|
||
| void updateDeleteLatestVersionTimestamp(Cell cell, Get get, int count, int maxVersions, |
There was a problem hiding this comment.
Had not done previously because original method modified was package private. Doing now.
| List<Cell> result = new ArrayList<>(); | ||
| result.addAll(deleteCells); |
There was a problem hiding this comment.
nit: we can wrap it with ArrayList constructor: new ArrayList<>(deleteCells);
| return 0; | ||
| }); | ||
| List<Cell> cells = new ArrayList<>(); | ||
| if (result.size() < count) { |
There was a problem hiding this comment.
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);
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
| 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); | ||
| } |
There was a problem hiding this comment.
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 :)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Addressed @virajjasani review comments. Fixed previous failed UTs, as those were validating the wrong behaviour. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| } | ||
|
|
||
| } else { | ||
| Cell getCell = result.get(0); |
There was a problem hiding this comment.
I am not sure if sorting of result is required here. If not required, rest looks good.
There was a problem hiding this comment.
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.
apache#2009) Signed-off-by: Viraj Jasani <vjasani@apache.org>
|
Oh, the PR has already been merged... I skimmed the comments on jira and also the patch, so the approach here is when we delete a specific version, we will go through all the old versions and added a delete marker for them if needed. This approach can solve a problem but will introduce other problems... |
I can revert it back. This has been a recurring source of confusion and complains among our customer base, and having inconsistent results prior and after compaction doesn't seem right, maybe we should make HBASE-15968 solution default here?
Yeah, but it's the same current behaviour for manually deleted ones, right? I guess both are solved by HBASE-15968 solution? |
Theoretically, HBASE-15968 can solve the problem. For the customers who have special usage, for example, set keepDeletedCells, need more than one versions, want to delete a specific version, and which is the most important, set the timestamp manually and the timestamp is not always increasing, the 'new behavior' can solve the problem, but usually it could also be solved by the customers themselves, by changing their usage to avoid hittin the bad cases... So if users meet the problem, I will suggest them to go to HBASE-15968, but usually, they will just change their usage... |
No, for the scenario below they are not the same. For example, you have t1 > t2 > t3, and you want to keep 2 versions. You delete t2, in the old behavior, t3 will come back, which is strange, so in your PR here, you also issue a delete for t3, to make sure it will not come back. But then if you put a new value with t3, in the old behavior, you can see the new value, but with the PR here, you can not see it, as there is a delete markar for t3... |
Ok, had reverted it from master.
Yeah, got your point, what I meant was that currently you already have same problem for the deleted t2, in your example, where you can't bring it back until the delete marker is gone. And for both cases, keepDeletedCells is a problem. There's another problem reported (not solved on this PR), when you put a Delete for a TS larger than all existing versions, it has actually no effects, not filtering out any of the older versions, because VERSION_DELETE requires delete TS equality, however, documentation states:
It could be fixed in a similar way, by fabricating individual delete markers for all existing versions older than the specified delete timestamp, or we can just assume this is correct, then update documentation accordingly. |
|
The only way to completely solve all the related problems is HBASE-15968, where we also consider MVCC so earlier modification will not effect later modifications. All other solutions can lead to inconsistent results under some strange scenarios... Agree that we should improve the document. I think we should provide some suggested ways to solve some strange behaviors. For example, the fix in this PR could also be done from client side right? And on HBASE-15968, let's wait some time for the input from @infraio . Thanks. |
Not used in our production cluster now...... |
Agreed. But we need more test and usage case for this feature. @wchevreuil Do you meet same case for this problem? You can take a try for it. It will be better if we can foucs on one same solution. And for the document, I am +1 to update it and make it more clearly. |
I agree we should aim towards a unique solution. Is HBASE-15968 not considered production ready because of performance concerns? And since "versions" feature behaves inconsistently without HBASE-15968, shouldn't we recall it as not production ready as well? I can work on documenting the inconsistencies caused by versions and related delete markers, meanwhile. |
5 participants
…s above VERSIONS limit
While going through some other delete marker issue, I had found out this issue is still present on master branch. Had rebased the original patch and pushed a PR.