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 modifying 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. Simply removing this check keeps things safe.
And I don't think the scenario
[here](https://github.com/apache/hbase/pull/7582#discussion_r2834656669)
warrants potential data loss. Even if this scenario did play out, the only
consequence is that we include a WAL that we don't technically need in a
backup. This is a non-issue, given that our restore process handles this
without any issues.
Finally, there was dicussion around trying to ensure "cross-table
consistency" and data being out of sync which I disagree with as well. This
backup is a point-in-time snapshot. In a distributed system, each RS is going
to have it's own point in time. This is similarly true for snapshots, where one
region's snapshot procedure can happen minutes prior to another one.
Regardless, I think preventing data loss scenarios and simplifying the code
should be the priority here.
--
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]
