HBASE-29261: Investigate flaw in backup deletion validation of PITR-critical backups and propose correct approach #6922
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anmolnar
left a comment
anmolnar
left a comment
There was a problem hiding this comment.
lgtm, but we need more unit tests to cover the different cases.
| System.out.println("Backup [" + targetBackup.getBackupId() | ||
| + "] covered PITR window for table [" + table + "]: " + coveredPitrWindow); |
There was a problem hiding this comment.
Are these stdout prints added to help development for now?
I don't think we should output it to the user.
There was a problem hiding this comment.
It was originally added to help during development, and I thought we could keep it.
But you're right — if it were a proper log message, it might have been acceptable, but plain printing isn't appropriate.
I'll remove it.
There was a problem hiding this comment.
I think this would be helpful as a log message. Maybe just as an INFO message.
There was a problem hiding this comment.
Is it possible to enabled debug logging for these commands?
If yes, emit at debug level instead.
There was a problem hiding this comment.
There's no logging currently enabled in that class, likely because these are user-facing commands where the intention is to directly output information rather than log it. Given that, I'll go ahead and skip adding a log statement here as well.
| System.out.println("Backup [" + backup.getBackupId() + "] covers the target window [" | ||
| + targetStart + ", " + targetEnd + "] with window [" + covered.getFirst() + ", " | ||
| + covered.getSecond() + "]"); |
There was a problem hiding this comment.
This looks like it could be a helpful log message as well.
There was a problem hiding this comment.
Ok, I don't mind, you can keep both if you guys believe they gonna be useful.
|
The logic looks good to me. Do you think it would make sense to add a unit test for each case you mentioned in your analysis write-up? |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Backup / restore related unit tests failed, do you work on those @vinayakphegde ? |
The failures are due to flaky/intermittent tests in incremental backup and are not related to the tests we’ve added. The upstream community and Ankit are already looking into it. |
…ritical backups and propose correct approach (#6922) * improve the logic of backup deletion validation of PITR-critical backups * add new tests
…ritical backups and propose correct approach (apache#6922) * improve the logic of backup deletion validation of PITR-critical backups * add new tests
…ritical backups and propose correct approach (apache#6922) * improve the logic of backup deletion validation of PITR-critical backups * add new tests
…ritical backups and propose correct approach (#6922) * improve the logic of backup deletion validation of PITR-critical backups * add new tests
…ritical backups and propose correct approach (#6922) * improve the logic of backup deletion validation of PITR-critical backups * add new tests
4 participants
The current approach incorrectly checks whether a full backup is the only valid base for all PITR target points, which is not a valid criterion. A full backup should not be required to support all PITR points to be considered necessary. Instead, each full backup only contributes to a specific PITR time range, depending on when the backup was taken and the availability of continuous backups afterward.
This ticket proposes a more accurate and conservative approach: