Skip to content
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

HBASE-28562 Correct backup ancestor calculation #5868

Merged
merged 5 commits into from
Jun 6, 2024
Merged

HBASE-28562 Correct backup ancestor calculation #5868

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7256354
HBASE-28562 Correct backup ancestor calculation
DieterDP-ng May 2, 2024
adc75b3
HBASE-28562 Correct backup ancestor calculation
DieterDP-ng May 3, 2024
89d764d
Run spotless:apply
DieterDP-ng May 3, 2024
3e8f2e9
Move BackupManager#getAncestors
DieterDP-ng May 15, 2024
cac1257
Process review comments.
DieterDP-ng May 30, 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
99 changes: 0 additions & 99 deletions hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.util.Map;
import java.util.Set;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.backup.BackupHFileCleaner;
Expand All @@ -34,8 +33,6 @@
import org.apache.hadoop.hbase.backup.BackupObserver;
import org.apache.hadoop.hbase.backup.BackupRestoreConstants;
import org.apache.hadoop.hbase.backup.BackupType;
import org.apache.hadoop.hbase.backup.HBackupFileSystem;
import org.apache.hadoop.hbase.backup.impl.BackupManifest.BackupImage;
import org.apache.hadoop.hbase.backup.master.BackupLogCleaner;
import org.apache.hadoop.hbase.backup.master.LogRollMasterProcedureManager;
import org.apache.hadoop.hbase.backup.regionserver.LogRollRegionServerProcedureManager;
Expand Down Expand Up @@ -268,102 +265,6 @@ public void setBackupInfo(BackupInfo backupInfo) {
this.backupInfo = backupInfo;
}

/**
* Get direct ancestors of the current backup.
* @param backupInfo The backup info for the current backup
* @return The ancestors for the current backup
* @throws IOException exception
*/
public ArrayList<BackupImage> getAncestors(BackupInfo backupInfo) throws IOException {
LOG.debug("Getting the direct ancestors of the current backup {}", backupInfo.getBackupId());

ArrayList<BackupImage> ancestors = new ArrayList<>();

// full backup does not have ancestor
if (backupInfo.getType() == BackupType.FULL) {
LOG.debug("Current backup is a full backup, no direct ancestor for it.");
return ancestors;
}

// get all backup history list in descending order
ArrayList<BackupInfo> allHistoryList = getBackupHistory(true);
for (BackupInfo backup : allHistoryList) {

BackupImage.Builder builder = BackupImage.newBuilder();

BackupImage image = builder.withBackupId(backup.getBackupId()).withType(backup.getType())
.withRootDir(backup.getBackupRootDir()).withTableList(backup.getTableNames())
.withStartTime(backup.getStartTs()).withCompleteTime(backup.getCompleteTs()).build();

// Only direct ancestors for a backup are required and not entire history of backup for this
// table resulting in verifying all of the previous backups which is unnecessary and backup
// paths need not be valid beyond the lifetime of a backup.
//
// RootDir is way of grouping a single backup including one full and many incremental backups
if (!image.getRootDir().equals(backupInfo.getBackupRootDir())) {
continue;
}

// add the full backup image as an ancestor until the last incremental backup
if (backup.getType().equals(BackupType.FULL)) {
// check the backup image coverage, if previous image could be covered by the newer ones,
// then no need to add
if (!BackupManifest.canCoverImage(ancestors, image)) {
ancestors.add(image);
}
} else {
// found last incremental backup, if previously added full backup ancestor images can cover
// it, then this incremental ancestor is not the dependent of the current incremental
// backup, that is to say, this is the backup scope boundary of current table set.
// Otherwise, this incremental backup ancestor is the dependent ancestor of the ongoing
// incremental backup
if (BackupManifest.canCoverImage(ancestors, image)) {
LOG.debug("Met the backup boundary of the current table set:");
for (BackupImage image1 : ancestors) {
LOG.debug(" BackupID={}, BackupDir={}", image1.getBackupId(), image1.getRootDir());
}
} else {
Path logBackupPath =
HBackupFileSystem.getBackupPath(backup.getBackupRootDir(), backup.getBackupId());
LOG.debug(
"Current backup has an incremental backup ancestor, "
+ "touching its image manifest in {}" + " to construct the dependency.",
logBackupPath.toString());
BackupManifest lastIncrImgManifest = new BackupManifest(conf, logBackupPath);
BackupImage lastIncrImage = lastIncrImgManifest.getBackupImage();
ancestors.add(lastIncrImage);

LOG.debug("Last dependent incremental backup image: {BackupID={}" + "BackupDir={}}",
lastIncrImage.getBackupId(), lastIncrImage.getRootDir());
}
}
}
LOG.debug("Got {} ancestors for the current backup.", ancestors.size());
return ancestors;
}

/**
* Get the direct ancestors of this backup for one table involved.
* @param backupInfo backup info
* @param table table
* @return backupImages on the dependency list
* @throws IOException exception
*/
public ArrayList<BackupImage> getAncestors(BackupInfo backupInfo, TableName table)
throws IOException {
ArrayList<BackupImage> ancestors = getAncestors(backupInfo);
ArrayList<BackupImage> tableAncestors = new ArrayList<>();
for (BackupImage image : ancestors) {
if (image.hasTable(table)) {
tableAncestors.add(image);
if (image.getType() == BackupType.FULL) {
break;
}
}
}
return tableAncestors;
}

/*
* backup system table operations
*/
Expand Down
98 changes: 0 additions & 98 deletions hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,104 +548,6 @@ public ArrayList<BackupImage> getDependentListByTable(TableName table) {
return tableImageList;
}

/**
* Get the full dependent image list in the whole dependency scope for a specific table of this
* backup in time order from old to new.
* @param table table
* @return the full backup image list for a table in time order in the whole scope of the
* dependency of this image
*/
public ArrayList<BackupImage> getAllDependentListByTable(TableName table) {
ArrayList<BackupImage> tableImageList = new ArrayList<>();
ArrayList<BackupImage> imageList = getRestoreDependentList(false);
for (BackupImage image : imageList) {
if (image.hasTable(table)) {
tableImageList.add(image);
}
}
return tableImageList;
}

/**
* Check whether backup image1 could cover backup image2 or not.
* @param image1 backup image 1
* @param image2 backup image 2
* @return true if image1 can cover image2, otherwise false
*/
public static boolean canCoverImage(BackupImage image1, BackupImage image2) {
// image1 can cover image2 only when the following conditions are satisfied:
// - image1 must not be an incremental image;
// - image1 must be taken after image2 has been taken;
// - table set of image1 must cover the table set of image2.
if (image1.getType() == BackupType.INCREMENTAL) {
return false;
}
if (image1.getStartTs() < image2.getStartTs()) {
return false;
}
List<TableName> image1TableList = image1.getTableNames();
List<TableName> image2TableList = image2.getTableNames();
boolean found;
for (int i = 0; i < image2TableList.size(); i++) {
found = false;
for (int j = 0; j < image1TableList.size(); j++) {
if (image2TableList.get(i).equals(image1TableList.get(j))) {
found = true;
break;
}
}
if (!found) {
return false;
}
}

LOG.debug("Backup image " + image1.getBackupId() + " can cover " + image2.getBackupId());
return true;
}

/**
* Check whether backup image set could cover a backup image or not.
* @param fullImages The backup image set
* @param image The target backup image
* @return true if fullImages can cover image, otherwise false
*/
public static boolean canCoverImage(ArrayList<BackupImage> fullImages, BackupImage image) {
// fullImages can cover image only when the following conditions are satisfied:
// - each image of fullImages must not be an incremental image;
// - each image of fullImages must be taken after image has been taken;
// - sum table set of fullImages must cover the table set of image.
for (BackupImage image1 : fullImages) {
if (image1.getType() == BackupType.INCREMENTAL) {
return false;
}
if (image1.getStartTs() < image.getStartTs()) {
return false;
}
}

ArrayList<String> image1TableList = new ArrayList<>();
for (BackupImage image1 : fullImages) {
List<TableName> tableList = image1.getTableNames();
for (TableName table : tableList) {
image1TableList.add(table.getNameAsString());
}
}
ArrayList<String> image2TableList = new ArrayList<>();
List<TableName> tableList = image.getTableNames();
for (TableName table : tableList) {
image2TableList.add(table.getNameAsString());
}

for (int i = 0; i < image2TableList.size(); i++) {
if (image1TableList.contains(image2TableList.get(i)) == false) {
return false;
}
}

LOG.debug("Full image set can cover image " + image.getBackupId());
return true;
}

public BackupInfo toBackupInfo() {
BackupInfo info = new BackupInfo();
info.setType(backupImage.getType());
Expand Down
2 changes: 1 addition & 1 deletion hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ public void execute() throws IOException {
backupManager.writeBackupStartCode(newStartCode);

// backup complete
completeBackup(conn, backupInfo, backupManager, BackupType.FULL, conf);
completeBackup(conn, backupInfo, BackupType.FULL, conf);
} catch (Exception e) {
failBackup(conn, backupInfo, backupManager, e, "Unexpected BackupException : ",
BackupType.FULL, conf);
Expand Down
2 changes: 1 addition & 1 deletion ...ackup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ public void execute() throws IOException {

handleBulkLoad(backupInfo.getTableNames());
// backup complete
completeBackup(conn, backupInfo, backupManager, BackupType.INCREMENTAL, conf);
completeBackup(conn, backupInfo, BackupType.INCREMENTAL, conf);

} catch (IOException e) {
failBackup(conn, backupInfo, backupManager, e, "Unexpected Exception : ",
Expand Down
67 changes: 61 additions & 6 deletions hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileStatus;
import org.apache.hadoop.fs.FileSystem;
Expand Down Expand Up @@ -271,8 +274,8 @@ public static void cleanupAndRestoreBackupSystem(Connection conn, BackupInfo bac
* @param backupInfo The current backup info
* @throws IOException exception
*/
protected void addManifest(BackupInfo backupInfo, BackupManager backupManager, BackupType type,
Configuration conf) throws IOException {
protected void addManifest(BackupInfo backupInfo, BackupType type, Configuration conf)
throws IOException {
// set the overall backup phase : store manifest
backupInfo.setPhase(BackupPhase.STORE_MANIFEST);

Expand All @@ -281,13 +284,65 @@ protected void addManifest(BackupInfo backupInfo, BackupManager backupManager, B
// set the table region server start and end timestamps for incremental backup
manifest.setIncrTimestampMap(backupInfo.getIncrTimestampMap());
}
ArrayList<BackupImage> ancestors = backupManager.getAncestors(backupInfo);
List<BackupImage> ancestors = getAncestors(backupInfo);
for (BackupImage image : ancestors) {
manifest.addDependentImage(image);
}
manifest.store(conf);
}

/**
* Gets the direct ancestors of the currently being created backup.
* @param backupInfo The backup info for the backup being created
*/
protected List<BackupImage> getAncestors(BackupInfo backupInfo) throws IOException {
LOG.debug("Getting the direct ancestors of the current backup {}", backupInfo.getBackupId());

// Full backups do not have ancestors
if (backupInfo.getType() == BackupType.FULL) {
LOG.debug("Current backup is a full backup, no direct ancestor for it.");
return Collections.emptyList();
}

List<BackupImage> ancestors = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Should the ancestors be a Set so that we don't end up with multiple entries for the same ancestor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a need for it. Since we're iterating over a list of unique possible ancestors, we can't end up with duplicates.

Set<TableName> tablesToCover = new HashSet<>(backupInfo.getTables());

// Go over the backup history list in descending order (newest to oldest)
DieterDP-ng marked this conversation as resolved.
Show resolved Hide resolved
List<BackupInfo> allHistoryList = backupManager.getBackupHistory(true);
Copy link
Member

Choose a reason for hiding this comment

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

Should this getBackupHistory() method support filters? Could we reduce the search space for applicable backups before entering this loop?

for (BackupInfo backup : allHistoryList) {
// If the image has a different rootDir, it cannot be an ancestor.
if (!backup.getBackupRootDir().equals(backupInfo.getBackupRootDir())) {
DieterDP-ng marked this conversation as resolved.
Show resolved Hide resolved
continue;
}

BackupImage.Builder builder = BackupImage.newBuilder();
BackupImage image = builder.withBackupId(backup.getBackupId()).withType(backup.getType())
.withRootDir(backup.getBackupRootDir()).withTableList(backup.getTableNames())
.withStartTime(backup.getStartTs()).withCompleteTime(backup.getCompleteTs()).build();

// The ancestors consist of the most recent FULL backups that cover the list of tables
// required in the new backup and all INCREMENTAL backups that came after one of those FULL
// backups.
if (backup.getType().equals(BackupType.INCREMENTAL)) {
ancestors.add(image);
LOG.debug("Dependent incremental backup image: {BackupID={}}", image.getBackupId());
} else {
if (tablesToCover.removeAll(image.getTableNames())) {
ancestors.add(image);
LOG.debug("Dependent full backup image: {BackupID={}}", image.getBackupId());

if (tablesToCover.isEmpty()) {
LOG.debug("Got {} ancestors for the current backup.", ancestors.size());
return ancestors;
DieterDP-ng marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

throw new IllegalStateException(
"Unable to find full backup that contains tables: " + tablesToCover);
}

/**
* Get backup request meta data dir as string.
* @param backupInfo backup info
Expand All @@ -312,15 +367,15 @@ protected String obtainBackupMetaDataStr(BackupInfo backupInfo) {
* @param backupInfo backup info
* @throws IOException exception
*/
protected void completeBackup(final Connection conn, BackupInfo backupInfo,
BackupManager backupManager, BackupType type, Configuration conf) throws IOException {
protected void completeBackup(final Connection conn, BackupInfo backupInfo, BackupType type,
Configuration conf) throws IOException {
// set the complete timestamp of the overall backup
backupInfo.setCompleteTs(EnvironmentEdgeManager.currentTime());
// set overall backup status: complete
backupInfo.setState(BackupState.COMPLETE);
backupInfo.setProgress(100);
// add and store the manifest for the backup
addManifest(backupInfo, backupManager, type, conf);
addManifest(backupInfo, type, conf);

// compose the backup complete data
String backupCompleteData =
Expand Down
4 changes: 2 additions & 2 deletions hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public void execute() throws IOException {
failStageIf(Stage.stage_4);

// backup complete
completeBackup(conn, backupInfo, backupManager, BackupType.INCREMENTAL, conf);
completeBackup(conn, backupInfo, BackupType.INCREMENTAL, conf);

} catch (Exception e) {
failBackup(conn, backupInfo, backupManager, e, "Unexpected Exception : ",
Expand Down Expand Up @@ -244,7 +244,7 @@ public void execute() throws IOException {
backupManager.writeBackupStartCode(newStartCode);
failStageIf(Stage.stage_4);
// backup complete
completeBackup(conn, backupInfo, backupManager, BackupType.FULL, conf);
completeBackup(conn, backupInfo, BackupType.FULL, conf);

} catch (Exception e) {

Expand Down
Loading