DieterDP-ng commented on code in PR #7582: URL: https://github.com/apache/hbase/pull/7582#discussion_r2676850468
########## hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java: ########## Review Comment: > No worries, appreciate you taking the time to look here. We've found a slew of bugs in the WAL retention system and I think it's important to get this right, so happy to iterate on feedback. > > > Since newTimestamps never is pruned, the entry in the backup table will keep growing over time. > > Agree with this. It's something we should take a look at. > > To your point about WAL retention and boundaries, conceptually, I've been trying to think about it from the perspective of "which WAL files have been backed up". Otherwise you run into issues when a host goes offline. > > For example, in the case where > > We have a Server A with row X > > 1. An incremental backup is taken, A is rolled > > 2. A writes more WAL files > > 3. Row X is deleted > > 4. A major compaction happens > > 5. A full backup is taken, WALs are rolled, but we don't update the timestamp for A. Row X is _not_ included in the full backup > > 6. An incremental backup is taken, we still have a very old roll time for this host. Row X is backed up again, and shows up in the backup even though we had previously deleted (but the tombstone no longer exists). > > > So we've resurfaced dead data that shouldn't be included. It's problematic to back up WALs that are very old. So this is the main culprit for the added complexity here I agree with your example, and agree that the change to `FullTableBackupClient` would fix this. It also shrinks the `newTimestamps`, which nice. > Additionally, I'm weary of comparing timestamps across hosts, which is why I was wary of doing something like generating a boundary timestamp in the backup process, which happens client side and opted to compare WAL timestamps which are generated by the same host. I see, I originally thought it might be less code to generate a client pre-roll timestamp, but it doesn't really simplify things. For the `FullTableBackupClient` at least, the current code is simple enough. I would suggest to split off a dedicated `calculateNewTimestamps` method with some proper javadoc. (Still thinking about the incremental case.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
