Skip to content

HBASE-28882 Backup restores are broken if the backup has moved locations #6294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ndimiduk merged 4 commits into from
Dec 3, 2024
Merged

HBASE-28882 Backup restores are broken if the backup has moved locations #6294

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e7cb4d
HBASE-28882 Backup restores are broken if the backup has moved locations
Sep 24, 2024
91871a4
Don't store BackupImage root dir in backup
Oct 9, 2024
d70c8dd
Deprecated javadoc
Nov 13, 2024
3e6d98a
Copy backup manifest on merge
Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...ackup/src/main/java/org/apache/hadoop/hbase/backup/mapreduce/MapReduceBackupMergeJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,15 @@ protected void copyMetaData(FileSystem fs, Path tmpBackupDir, Path backupDirPath
if (
fileName.indexOf(FSTableDescriptors.TABLEINFO_DIR) > 0
|| fileName.indexOf(HRegionFileSystem.REGION_INFO_FILE) > 0
|| fileName.indexOf(BackupManifest.MANIFEST_FILE_NAME) > 0
Copy link
Contributor Author

@rmdmattingly rmdmattingly Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This becomes necessary because the manifest file would implicitly move to the tmp directory during the merge process, since it follows the application's working root dir rather than its original root dir. So, before this changeset, rewriting the manifest would always update the original root dir (where we want this eventually), but with this changeset rewriting the manifest would put it in the tmp dir. Since the tmp dir will be purged, we need to make sure we copy the manifest file out first

Copy link
Member

@ndimiduk ndimiduk Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test that asserts that we haven't lost any critical components of a backup image during processing it?

Copy link
Contributor Author

@rmdmattingly rmdmattingly Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so specifically I suppose. But the larger tests do implicitly validate that — for example, I realized this change was necessary due to test failures

Copy link
Contributor

@DieterDP-ng DieterDP-ng Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with this change, I can't find what was changed that previously put the manifest in the correct location. Just curious.

Copy link
Contributor Author

@rmdmattingly rmdmattingly Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, this was confusing to me too. Before we stripped the root directory from the BackupImage, rewriting the manifest would implicitly always use the original root directory. So this only worked because we were unable to relocate backups

) {
toKeep.add(p);
}
}
// Copy meta to destination
for (Path p : toKeep) {
Path newPath = convertToDest(p, backupDirPath);
LOG.info("Copying tmp metadata from {} to {}", p, newPath);
copyFile(fs, p, newPath);
}
}
Expand Down Expand Up @@ -310,8 +312,11 @@ protected void updateBackupManifest(String backupRoot, String mergedBackupId,
List<String> backupsToDelete) throws IllegalArgumentException, IOException {
BackupManifest manifest =
HBackupFileSystem.getManifest(conf, new Path(backupRoot), mergedBackupId);
LOG.info("Removing ancestors from merged backup {} : {}", mergedBackupId, backupsToDelete);
manifest.getBackupImage().removeAncestors(backupsToDelete);
// save back
LOG.info("Creating new manifest file for merged backup {} at root {}", mergedBackupId,
backupRoot);
manifest.store(conf);
}

Expand All @@ -320,12 +325,14 @@ protected void deleteBackupImages(List<String> backupIds, Connection conn, FileS
// Delete from backup system table
try (BackupSystemTable table = new BackupSystemTable(conn)) {
for (String backupId : backupIds) {
LOG.info("Removing metadata for backup {}", backupId);
table.deleteBackupInfo(backupId);
}
}

// Delete from file system
for (String backupId : backupIds) {
LOG.info("Purging backup {} from FileSystem", backupId);
Path backupDirPath = HBackupFileSystem.getBackupPath(backupRoot, backupId);

if (!fs.delete(backupDirPath, true)) {
Expand Down