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.

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-28568 Fix incremental backup set shrinking #5876

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ public BackupInfo getBackupInfo(String backupId) throws IOException {
public int deleteBackups(String[] backupIds) throws IOException {

int totalDeleted = 0;
Map<String, HashSet<TableName>> allTablesMap = new HashMap<>();

boolean deleteSessionStarted;
boolean snapshotDone;
Expand Down Expand Up @@ -129,20 +128,15 @@ public int deleteBackups(String[] backupIds) throws IOException {
}
snapshotDone = true;
Copy link
Member

Choose a reason for hiding this comment

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

That snapshot logic looks problematic. Is it okay to continue using an existing snapshot? If an existing snapshot exists then we don't know what it contains and this operation should fail. Separate issue I guess?

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 agree it could be problematic. Perhaps the backup repair takes care of this kind of situations, I'm not sure.
But this is a separate issue indeed.

Copy link
Member

Choose a reason for hiding this comment

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

FYI @rmdmattingly this is one to look into.

try {
List<String> affectedBackupRootDirs = new ArrayList<>();
for (int i = 0; i < backupIds.length; i++) {
BackupInfo info = sysTable.readBackupInfo(backupIds[i]);
if (info != null) {
DieterDP-ng marked this conversation as resolved.
Show resolved Hide resolved
String rootDir = info.getBackupRootDir();
HashSet<TableName> allTables = allTablesMap.get(rootDir);
if (allTables == null) {
allTables = new HashSet<>();
allTablesMap.put(rootDir, allTables);
}
allTables.addAll(info.getTableNames());
affectedBackupRootDirs.add(info.getBackupRootDir());
totalDeleted += deleteBackup(backupIds[i], sysTable);
}
}
finalizeDelete(allTablesMap, sysTable);
finalizeDelete(affectedBackupRootDirs, sysTable);
// Finish
sysTable.finishDeleteOperation();
// delete snapshot
Expand Down Expand Up @@ -175,26 +169,23 @@ public int deleteBackups(String[] backupIds) throws IOException {

/**
* Updates incremental backup set for every backupRoot
* @param tablesMap map [backupRoot: {@code Set<TableName>}]
* @param table backup system table
* @param backupRoots backupRoots for which to revise the incremental backup set
* @param table backup system table
* @throws IOException if a table operation fails
*/
private void finalizeDelete(Map<String, HashSet<TableName>> tablesMap, BackupSystemTable table)
private void finalizeDelete(List<String> backupRoots, BackupSystemTable table)
throws IOException {
for (String backupRoot : tablesMap.keySet()) {
for (String backupRoot : backupRoots) {
Set<TableName> incrTableSet = table.getIncrementalBackupTableSet(backupRoot);
Map<TableName, ArrayList<BackupInfo>> tableMap =
Map<TableName, List<BackupInfo>> tableMap =
table.getBackupHistoryForTableSet(incrTableSet, backupRoot);
for (Map.Entry<TableName, ArrayList<BackupInfo>> entry : tableMap.entrySet()) {
if (entry.getValue() == null) {
// No more backups for a table
incrTableSet.remove(entry.getKey());
}
}

// Keep only the tables that are present in other backups
incrTableSet.retainAll(tableMap.keySet());

table.deleteIncrementalBackupTableSet(backupRoot);
Copy link
Member

Choose a reason for hiding this comment

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

This API on BackupSystemTable is pretty ham-handed. Would be better if we could delete on the table entries that are being removed, rather than deleting and re-creating the row. I assume the lock taken via startBackupExclusiveOperation() at the beginning of deleteBackups prevents another actor from modifying that row concurrently...

if (!incrTableSet.isEmpty()) {
table.addIncrementalBackupTableSet(incrTableSet, backupRoot);
} else { // empty
table.deleteIncrementalBackupTableSet(backupRoot);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -839,23 +839,25 @@ public List<BackupInfo> getBackupHistoryForTable(TableName name) throws IOExcept
return tableHistory;
}

public Map<TableName, ArrayList<BackupInfo>> getBackupHistoryForTableSet(Set<TableName> set,
/**
* Goes through all backup history corresponding to the provided root folder, and collects all
* backup info mentioning each of the provided tables.
* @param set the tables for which to collect the {@code BackupInfo}
* @param backupRoot backup destination path to retrieve backup history for
* @return a map containing (a subset of) the provided {@code TableName}s, mapped to a list of at
* least one {@code BackupInfo}
* @throws IOException if getting the backup history fails
*/
public Map<TableName, List<BackupInfo>> getBackupHistoryForTableSet(Set<TableName> set,
String backupRoot) throws IOException {
List<BackupInfo> history = getBackupHistory(backupRoot);
Map<TableName, ArrayList<BackupInfo>> tableHistoryMap = new HashMap<>();
for (Iterator<BackupInfo> iterator = history.iterator(); iterator.hasNext();) {
BackupInfo info = iterator.next();
if (!backupRoot.equals(info.getBackupRootDir())) {
continue;
}
Map<TableName, List<BackupInfo>> tableHistoryMap = new HashMap<>();
for (BackupInfo info : history) {
List<TableName> tables = info.getTableNames();
for (TableName tableName : tables) {
if (set.contains(tableName)) {
ArrayList<BackupInfo> list = tableHistoryMap.get(tableName);
if (list == null) {
list = new ArrayList<>();
tableHistoryMap.put(tableName, list);
}
List<BackupInfo> list =
tableHistoryMap.computeIfAbsent(tableName, k -> new ArrayList<>());
list.add(info);
}
}
Expand Down