hgromer commented on code in PR #8694:
URL: https://github.com/apache/hbase/pull/8694#discussion_r4085063117
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -217,14 +251,6 @@ private List<String> getLogFilesForNewBackup(Map<String,
Long> olderTimestamps,
resultLogFiles.add(currentLogFile);
}
- // It is possible that a host in .oldlogs is an obsolete region server
- // so newestTimestamps.get(host) here can be null.
- // Even if these logs belong to a obsolete region server, we still need
- // to include they to avoid loss of edits for backup.
- Long newTimestamp = newestTimestamps.get(host);
Review Comment:
I know there was debate whether this was necessary or not
[here](https://github.com/apache/hbase/pull/7582#discussion_r2665468836), and
want to emphasize that this is a very important change. Without this change,
we're subject to data loss anytime a new RS joins the cluster.
If we add just this code snippet back, our `TestBackupOfflineRS` tests will
fail, showing this is in fact problematic. Importantly, adding a file to
`newestLog` means it _is not_ included in the backup. The roll time moves
forward, and then the log cleaner deletes these logs without them ever being
included in the backup.
There was some discussion about transforming this check due to hypothetical
scenarios. I think it's a lot safer to simply remove this block which can cause
data loss and provides no value.
--
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]
