From 725635441f69f18d7f1942d96d019922da5823b3 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Thu, 2 May 2024 17:55:49 +0200 Subject: [PATCH 1/5] HBASE-28562 Correct backup ancestor calculation The ancestor calculation was wrong for incremental backups: when requesting the ancestors for an incremental backup X, the ancestors could include both full and incremental backups that predate the full backup on which X is built. This caused a crash in incremental backup creation when data of old incremental backups was deleted through other means than the HBase API (i.e. without the HBase backup system table being updated). --- .../hbase/backup/impl/BackupManager.java | 45 +---------- .../TestIncrementalBackupWithDataLoss.java | 79 +++++++++++++++++++ 2 files changed, 83 insertions(+), 41 deletions(-) create mode 100644 hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java index ed1755ad5021..a95bde93fd0d 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java @@ -285,7 +285,7 @@ public ArrayList getAncestors(BackupInfo backupInfo) throws IOExcep return ancestors; } - // get all backup history list in descending order + // get all backup history list in descending order (newest to oldest) ArrayList allHistoryList = getBackupHistory(true); for (BackupInfo backup : allHistoryList) { @@ -295,47 +295,10 @@ public ArrayList getAncestors(BackupInfo backupInfo) throws IOExcep .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 + LOG.debug("Dependent incremental backup image: {BackupID={}}", image.getBackupId()); + ancestors.add(image); 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()); - } + break; } } LOG.debug("Got {} ancestors for the current backup.", ancestors.size()); diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java new file mode 100644 index 000000000000..b286e88682ae --- /dev/null +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.backup; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.backup.impl.BackupAdminImpl; +import org.apache.hadoop.hbase.client.Connection; +import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +import static org.junit.Assert.assertTrue; + +@Category(LargeTests.class) +public class TestIncrementalBackupWithDataLoss extends TestBackupBase { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestIncrementalBackupWithDataLoss.class); + + private static final Logger LOG = LoggerFactory.getLogger(TestIncrementalBackupWithDataLoss.class); + + @Test + public void testFullBackupBreaksDependencyOnOlderBackups() throws Exception { + LOG.info("test creation of backups after backup data was lost"); + + try (Connection conn = ConnectionFactory.createConnection(conf1)) { + BackupAdminImpl client = new BackupAdminImpl(conn); + List tables = Lists.newArrayList(table1); + + insertIntoTable(conn, table1, famName, 1, 1).close(); + String backup1 = client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); + insertIntoTable(conn, table1, famName, 2, 1).close(); + String backup2 = client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + + assertTrue(checkSucceeded(backup1)); + assertTrue(checkSucceeded(backup2)); + + // Simulate data loss on the backup storage + TEST_UTIL.getTestFileSystem().delete(new Path(BACKUP_ROOT_DIR, backup2), true); + + insertIntoTable(conn, table1, famName, 4, 1).close(); + String backup4 = client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); + insertIntoTable(conn, table1, famName, 5, 1).close(); + String backup5 = client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + insertIntoTable(conn, table1, famName, 6, 1).close(); + String backup6 = client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + + assertTrue(checkSucceeded(backup4)); + assertTrue(checkSucceeded(backup5)); + assertTrue(checkSucceeded(backup6)); + } + } + +} From adc75b325e7c7bb2bb5b6f0225a233002d52a176 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Fri, 3 May 2024 17:13:44 +0200 Subject: [PATCH 2/5] HBASE-28562 Correct backup ancestor calculation The ancestor calculation was wrong for incremental backups: when requesting the ancestors for an incremental backup X, the ancestors could include both full and incremental backups that predate the full backup on which X is built. This caused a crash in incremental backup creation when data of old incremental backups was deleted through other means than the HBase API (i.e. without the HBase backup system table being updated). --- .../hbase/backup/impl/BackupManager.java | 32 ++++-- .../hbase/backup/impl/BackupManifest.java | 98 ------------------- 2 files changed, 26 insertions(+), 104 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java index a95bde93fd0d..a615e1ed3587 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -285,6 +286,8 @@ public ArrayList getAncestors(BackupInfo backupInfo) throws IOExcep return ancestors; } + Set tablesToCover = new HashSet<>(backupInfo.getTables()); + // get all backup history list in descending order (newest to oldest) ArrayList allHistoryList = getBackupHistory(true); for (BackupInfo backup : allHistoryList) { @@ -295,14 +298,31 @@ public ArrayList getAncestors(BackupInfo backupInfo) throws IOExcep .withRootDir(backup.getBackupRootDir()).withTableList(backup.getTableNames()) .withStartTime(backup.getStartTs()).withCompleteTime(backup.getCompleteTs()).build(); - LOG.debug("Dependent incremental backup image: {BackupID={}}", image.getBackupId()); - ancestors.add(image); - if (backup.getType().equals(BackupType.FULL)) { - break; + // If the image has a different rootDir, it cannot be an ancestor. + if (!image.getRootDir().equals(backupInfo.getBackupRootDir())) { + continue; + } + + // 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; + } + } } } - LOG.debug("Got {} ancestors for the current backup.", ancestors.size()); - return ancestors; + + throw new IllegalStateException("Unable to find full backup that contains tables: " + tablesToCover); } /** diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java index 237d8686ab79..d66b5886794c 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java @@ -548,104 +548,6 @@ public ArrayList 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 getAllDependentListByTable(TableName table) { - ArrayList tableImageList = new ArrayList<>(); - ArrayList 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 image1TableList = image1.getTableNames(); - List 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 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 image1TableList = new ArrayList<>(); - for (BackupImage image1 : fullImages) { - List tableList = image1.getTableNames(); - for (TableName table : tableList) { - image1TableList.add(table.getNameAsString()); - } - } - ArrayList image2TableList = new ArrayList<>(); - List 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()); From 89d764d5a594832ad36a7d3b336b8b5fbae8b5f6 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Fri, 3 May 2024 20:56:30 +0200 Subject: [PATCH 3/5] Run spotless:apply --- .../hbase/backup/impl/BackupManager.java | 5 ++-- .../TestIncrementalBackupWithDataLoss.java | 26 ++++++++++++------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java index a615e1ed3587..91c609557d5d 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java @@ -26,7 +26,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; @@ -35,7 +34,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; @@ -322,7 +320,8 @@ public ArrayList getAncestors(BackupInfo backupInfo) throws IOExcep } } - throw new IllegalStateException("Unable to find full backup that contains tables: " + tablesToCover); + throw new IllegalStateException( + "Unable to find full backup that contains tables: " + tablesToCover); } /** diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java index b286e88682ae..cf442f5f0dd7 100644 --- a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupWithDataLoss.java @@ -17,6 +17,9 @@ */ package org.apache.hadoop.hbase.backup; +import static org.junit.Assert.assertTrue; + +import java.util.List; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.TableName; @@ -24,16 +27,13 @@ import org.apache.hadoop.hbase.client.Connection; import org.apache.hadoop.hbase.client.ConnectionFactory; import org.apache.hadoop.hbase.testclassification.LargeTests; -import org.apache.hbase.thirdparty.com.google.common.collect.Lists; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.List; - -import static org.junit.Assert.assertTrue; +import org.apache.hbase.thirdparty.com.google.common.collect.Lists; @Category(LargeTests.class) public class TestIncrementalBackupWithDataLoss extends TestBackupBase { @@ -42,7 +42,8 @@ public class TestIncrementalBackupWithDataLoss extends TestBackupBase { public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestIncrementalBackupWithDataLoss.class); - private static final Logger LOG = LoggerFactory.getLogger(TestIncrementalBackupWithDataLoss.class); + private static final Logger LOG = + LoggerFactory.getLogger(TestIncrementalBackupWithDataLoss.class); @Test public void testFullBackupBreaksDependencyOnOlderBackups() throws Exception { @@ -53,9 +54,11 @@ public void testFullBackupBreaksDependencyOnOlderBackups() throws Exception { List tables = Lists.newArrayList(table1); insertIntoTable(conn, table1, famName, 1, 1).close(); - String backup1 = client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); + String backup1 = + client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); insertIntoTable(conn, table1, famName, 2, 1).close(); - String backup2 = client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + String backup2 = + client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); assertTrue(checkSucceeded(backup1)); assertTrue(checkSucceeded(backup2)); @@ -64,11 +67,14 @@ public void testFullBackupBreaksDependencyOnOlderBackups() throws Exception { TEST_UTIL.getTestFileSystem().delete(new Path(BACKUP_ROOT_DIR, backup2), true); insertIntoTable(conn, table1, famName, 4, 1).close(); - String backup4 = client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); + String backup4 = + client.backupTables(createBackupRequest(BackupType.FULL, tables, BACKUP_ROOT_DIR)); insertIntoTable(conn, table1, famName, 5, 1).close(); - String backup5 = client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + String backup5 = + client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); insertIntoTable(conn, table1, famName, 6, 1).close(); - String backup6 = client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); + String backup6 = + client.backupTables(createBackupRequest(BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR)); assertTrue(checkSucceeded(backup4)); assertTrue(checkSucceeded(backup5)); From 3e8f2e9a2abda8467eedd01111495ec1b3e11a25 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Wed, 15 May 2024 22:43:24 +0200 Subject: [PATCH 4/5] Move BackupManager#getAncestors Move this method since it is only used by the TableBackupClient. --- .../hbase/backup/impl/BackupManager.java | 81 ------------------- .../backup/impl/FullTableBackupClient.java | 2 +- .../impl/IncrementalTableBackupClient.java | 2 +- .../hbase/backup/impl/TableBackupClient.java | 67 +++++++++++++-- .../hadoop/hbase/backup/TestBackupBase.java | 4 +- 5 files changed, 65 insertions(+), 91 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java index 91c609557d5d..f0c93db4b4c2 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java @@ -21,7 +21,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -34,7 +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.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; @@ -267,85 +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 getAncestors(BackupInfo backupInfo) throws IOException { - LOG.debug("Getting the direct ancestors of the current backup {}", backupInfo.getBackupId()); - - ArrayList 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; - } - - Set tablesToCover = new HashSet<>(backupInfo.getTables()); - - // get all backup history list in descending order (newest to oldest) - ArrayList 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(); - - // If the image has a different rootDir, it cannot be an ancestor. - if (!image.getRootDir().equals(backupInfo.getBackupRootDir())) { - continue; - } - - // 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; - } - } - } - } - - throw new IllegalStateException( - "Unable to find full backup that contains tables: " + tablesToCover); - } - - /** - * 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 getAncestors(BackupInfo backupInfo, TableName table) - throws IOException { - ArrayList ancestors = getAncestors(backupInfo); - ArrayList 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 */ diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java index fee2e825728e..06dad8880b5b 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java @@ -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); diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java index 211e9f96c89c..b7d1c4a95cc6 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalTableBackupClient.java @@ -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 : ", diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java index e758ced3f846..6b2ec938a457 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java @@ -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; @@ -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); @@ -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 ancestors = backupManager.getAncestors(backupInfo); + List 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 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 ancestors = new ArrayList<>(); + Set tablesToCover = new HashSet<>(backupInfo.getTables()); + + // Go over the backup history list in descending order (newest to oldest) + List allHistoryList = backupManager.getBackupHistory(true); + for (BackupInfo backup : allHistoryList) { + // If the image has a different rootDir, it cannot be an ancestor. + if (!backup.getBackupRootDir().equals(backupInfo.getBackupRootDir())) { + 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; + } + } + } + } + + throw new IllegalStateException( + "Unable to find full backup that contains tables: " + tablesToCover); + } + /** * Get backup request meta data dir as string. * @param backupInfo backup info @@ -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 = diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java index 7b5095a897e2..e9c1cfd9c323 100644 --- a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupBase.java @@ -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 : ", @@ -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) { From cac1257a2eb6e8c1377823cb632c1a0647b0f5a2 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Thu, 30 May 2024 10:43:01 +0200 Subject: [PATCH 5/5] Process review comments. --- .../hadoop/hbase/backup/impl/TableBackupClient.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java index 6b2ec938a457..0aa6516fe4f3 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java @@ -23,6 +23,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; @@ -307,11 +308,11 @@ protected List getAncestors(BackupInfo backupInfo) throws IOExcepti List ancestors = new ArrayList<>(); Set tablesToCover = new HashSet<>(backupInfo.getTables()); - // Go over the backup history list in descending order (newest to oldest) + // Go over the backup history list from newest to oldest List allHistoryList = backupManager.getBackupHistory(true); for (BackupInfo backup : allHistoryList) { // If the image has a different rootDir, it cannot be an ancestor. - if (!backup.getBackupRootDir().equals(backupInfo.getBackupRootDir())) { + if (!Objects.equals(backup.getBackupRootDir(), backupInfo.getBackupRootDir())) { continue; } @@ -327,13 +328,13 @@ protected List getAncestors(BackupInfo backupInfo) throws IOExcepti ancestors.add(image); LOG.debug("Dependent incremental backup image: {BackupID={}}", image.getBackupId()); } else { - if (tablesToCover.removeAll(image.getTableNames())) { + if (tablesToCover.removeAll(new HashSet<>(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; + return Collections.unmodifiableList(ancestors); } } }