This is an automated email from the ASF dual-hosted git repository.
ndimiduk pushed a commit to branch branch-2.6
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2.6 by this push:
new baabc1d7c4e HBASE-28882 Backup restores are broken if the backup has
moved locations (#6294)
baabc1d7c4e is described below
commit baabc1d7c4e639a8e9a5838ff14494b75a769522
Author: Ray Mattingly <[email protected]>
AuthorDate: Tue Dec 3 09:10:35 2024 -0500
HBASE-28882 Backup restores are broken if the backup has moved locations
(#6294)
Co-authored-by: Ray Mattingly <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Reviewed-by: Dieter De Paepe <[email protected]>
---
.../org/apache/hadoop/hbase/backup/BackupInfo.java | 2 +-
.../hadoop/hbase/backup/HBackupFileSystem.java | 22 +++++++++-
.../hadoop/hbase/backup/impl/BackupManifest.java | 24 ++++++++++-
.../backup/mapreduce/MapReduceBackupMergeJob.java | 7 ++++
.../hadoop/hbase/backup/TestHBackupFileSystem.java | 48 ++++++++++++++++++++++
.../hadoop/hbase/backup/TestIncrementalBackup.java | 24 ++++++++++-
.../src/main/protobuf/Backup.proto | 3 +-
7 files changed, 124 insertions(+), 6 deletions(-)
diff --git
a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
index 5a0094740d6..f0dc10b8361 100644
--- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
+++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
@@ -267,7 +267,7 @@ public class BackupInfo implements Comparable<BackupInfo> {
}
public void setFailedMsg(String failedMsg) {
- if (failedMsg.length() > MAX_FAILED_MESSAGE_LENGTH) {
+ if (failedMsg != null && failedMsg.length() > MAX_FAILED_MESSAGE_LENGTH) {
failedMsg = failedMsg.substring(0, MAX_FAILED_MESSAGE_LENGTH);
}
this.failedMsg = failedMsg;
diff --git
a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/HBackupFileSystem.java
b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/HBackupFileSystem.java
index 2b27e752747..d5fd9aaf4c3 100644
---
a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/HBackupFileSystem.java
+++
b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/HBackupFileSystem.java
@@ -17,6 +17,7 @@
*/
package org.apache.hadoop.hbase.backup;
+import com.google.errorprone.annotations.RestrictedApi;
import java.io.IOException;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
@@ -27,6 +28,8 @@ import org.apache.yetus.audience.InterfaceAudience;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
+
/**
* View to an on-disk Backup Image FileSytem Provides the set of methods
necessary to interact with
* the on-disk Backup Image data.
@@ -97,10 +100,18 @@ public final class HBackupFileSystem {
private static Path getManifestPath(Configuration conf, Path backupRootPath,
String backupId)
throws IOException {
+ return getManifestPath(conf, backupRootPath, backupId, true);
+ }
+
+ /* Visible for testing only */
+ @RestrictedApi(explanation = "Should only be called internally or in tests",
link = "",
+ allowedOnPath =
"(.*/src/test/.*|.*/org/apache/hadoop/hbase/backup/HBackupFileSystem.java)")
+ static Path getManifestPath(Configuration conf, Path backupRootPath, String
backupId,
+ boolean throwIfNotFound) throws IOException {
FileSystem fs = backupRootPath.getFileSystem(conf);
Path manifestPath = new Path(getBackupPath(backupRootPath.toString(),
backupId) + Path.SEPARATOR
+ BackupManifest.MANIFEST_FILE_NAME);
- if (!fs.exists(manifestPath)) {
+ if (throwIfNotFound && !fs.exists(manifestPath)) {
String errorMsg = "Could not find backup manifest " +
BackupManifest.MANIFEST_FILE_NAME
+ " for " + backupId + ". File " + manifestPath + " does not exists.
Did " + backupId
+ " correspond to previously taken backup ?";
@@ -109,6 +120,15 @@ public final class HBackupFileSystem {
return manifestPath;
}
+ public static Path getRootDirFromBackupPath(Path backupPath, String
backupId) {
+ if (backupPath.getName().equals(BackupManifest.MANIFEST_FILE_NAME)) {
+ backupPath = backupPath.getParent();
+ }
+ Preconditions.checkArgument(backupPath.getName().equals(backupId),
+ String.format("Backup path %s must end in backupId %s", backupPath,
backupId));
+ return backupPath.getParent();
+ }
+
public static BackupManifest getManifest(Configuration conf, Path
backupRootPath, String backupId)
throws IOException {
BackupManifest manifest =
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 d66b5886794..59ae3857f2e 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
@@ -17,6 +17,7 @@
*/
package org.apache.hadoop.hbase.backup.impl;
+import com.google.errorprone.annotations.RestrictedApi;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
@@ -158,12 +159,17 @@ public class BackupManifest {
return image;
}
+ /**
+ * This method deliberately does not include the backup root dir on the
produced proto. This is
+ * because we don't want to persist the root dir on the backup itself, so
that backups can still
+ * be used after they have moved locations. A restore's operator will
always provide the root
+ * dir.
+ */
BackupProtos.BackupImage toProto() {
BackupProtos.BackupImage.Builder builder =
BackupProtos.BackupImage.newBuilder();
builder.setBackupId(backupId);
builder.setCompleteTs(completeTs);
builder.setStartTs(startTs);
- builder.setBackupRootDir(rootDir);
if (type == BackupType.FULL) {
builder.setBackupType(BackupProtos.BackupType.FULL);
} else {
@@ -439,7 +445,7 @@ public class BackupManifest {
} catch (Exception e) {
throw new BackupException(e);
}
- this.backupImage = BackupImage.fromProto(proto);
+ this.backupImage = hydrateRootDir(BackupImage.fromProto(proto),
backupPath);
LOG.debug("Loaded manifest instance from manifest file: "
+ BackupUtils.getPath(subFile.getPath()));
return;
@@ -452,6 +458,20 @@ public class BackupManifest {
}
}
+ /* Visible for testing only */
+ @RestrictedApi(explanation = "Should only be called internally or in tests",
link = "",
+ allowedOnPath =
"(.*/src/test/.*|.*/org/apache/hadoop/hbase/backup/impl/BackupManifest.java)")
+ public static BackupImage hydrateRootDir(BackupImage backupImage, Path
backupPath)
+ throws IOException {
+ String providedRootDir =
+ HBackupFileSystem.getRootDirFromBackupPath(backupPath,
backupImage.backupId).toString();
+ backupImage.setRootDir(providedRootDir);
+ for (BackupImage ancestor : backupImage.getAncestors()) {
+ ancestor.setRootDir(providedRootDir);
+ }
+ return backupImage;
+ }
+
public BackupType getType() {
return backupImage.getType();
}
diff --git
a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/mapreduce/MapReduceBackupMergeJob.java
b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/mapreduce/MapReduceBackupMergeJob.java
index 1f9ae16c1df..0549a3371cf 100644
---
a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/mapreduce/MapReduceBackupMergeJob.java
+++
b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/mapreduce/MapReduceBackupMergeJob.java
@@ -228,6 +228,7 @@ public class MapReduceBackupMergeJob implements
BackupMergeJob {
if (
fileName.indexOf(FSTableDescriptors.TABLEINFO_DIR) > 0
|| fileName.indexOf(HRegionFileSystem.REGION_INFO_FILE) > 0
+ || fileName.indexOf(BackupManifest.MANIFEST_FILE_NAME) > 0
) {
toKeep.add(p);
}
@@ -235,6 +236,7 @@ public class MapReduceBackupMergeJob implements
BackupMergeJob {
// 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);
}
}
@@ -310,8 +312,11 @@ public class MapReduceBackupMergeJob implements
BackupMergeJob {
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);
}
@@ -320,12 +325,14 @@ public class MapReduceBackupMergeJob implements
BackupMergeJob {
// 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)) {
diff --git
a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestHBackupFileSystem.java
b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestHBackupFileSystem.java
new file mode 100644
index 00000000000..3bdc2f62079
--- /dev/null
+++
b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestHBackupFileSystem.java
@@ -0,0 +1,48 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(SmallTests.class)
+public class TestHBackupFileSystem {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestHBackupFileSystem.class);
+
+ private static final Path ROOT_DIR = new Path("/backup/root");
+ private static final String BACKUP_ID = "123";
+
+ @Test
+ public void testRootDirManifestPathConversion() throws IOException {
+ Path manifestPath =
+ HBackupFileSystem.getManifestPath(new Configuration(), ROOT_DIR,
BACKUP_ID, false);
+ Path convertedRootDir =
HBackupFileSystem.getRootDirFromBackupPath(manifestPath, BACKUP_ID);
+ assertEquals(ROOT_DIR, convertedRootDir);
+ }
+}
diff --git
a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackup.java
b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackup.java
index 1e43b2aeb72..fce64c276e6 100644
---
a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackup.java
+++
b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackup.java
@@ -106,7 +106,10 @@ public class TestIncrementalBackup extends TestBackupBase {
insertIntoTable(conn, table1, mobName, 3, NB_ROWS_FAM3).close();
Admin admin = conn.getAdmin();
BackupAdminImpl client = new BackupAdminImpl(conn);
+ BackupRequest request = createBackupRequest(BackupType.FULL, tables,
BACKUP_ROOT_DIR);
String backupIdFull = takeFullBackup(tables, client);
+ validateRootPathCanBeOverridden(BACKUP_ROOT_DIR, backupIdFull);
+ assertTrue(checkSucceeded(backupIdFull));
// #2 - insert some data to table
Table t1 = insertIntoTable(conn, table1, famName, 1, ADD_ROWS);
@@ -148,12 +151,13 @@ public class TestIncrementalBackup extends TestBackupBase
{
// #3 - incremental backup for multiple tables
tables = Lists.newArrayList(table1, table2);
- BackupRequest request = createBackupRequest(BackupType.INCREMENTAL,
tables, BACKUP_ROOT_DIR);
+ request = createBackupRequest(BackupType.INCREMENTAL, tables,
BACKUP_ROOT_DIR);
String backupIdIncMultiple = client.backupTables(request);
assertTrue(checkSucceeded(backupIdIncMultiple));
BackupManifest manifest =
HBackupFileSystem.getManifest(conf1, new Path(BACKUP_ROOT_DIR),
backupIdIncMultiple);
assertEquals(Sets.newHashSet(table1, table2), new
HashSet<>(manifest.getTableList()));
+ validateRootPathCanBeOverridden(BACKUP_ROOT_DIR, backupIdIncMultiple);
// add column family f2 to table1
// drop column family f3
@@ -181,6 +185,7 @@ public class TestIncrementalBackup extends TestBackupBase {
request = createBackupRequest(BackupType.INCREMENTAL, tables,
BACKUP_ROOT_DIR);
String backupIdIncMultiple2 = client.backupTables(request);
assertTrue(checkSucceeded(backupIdIncMultiple2));
+ validateRootPathCanBeOverridden(BACKUP_ROOT_DIR, backupIdIncMultiple2);
// #5 - restore full backup for all tables
TableName[] tablesRestoreFull = new TableName[] { table1, table2 };
@@ -247,4 +252,21 @@ public class TestIncrementalBackup extends TestBackupBase {
checkSucceeded(backupId);
return backupId;
}
+
+ /**
+ * Check that backup manifest can be produced for a different root. Users
may want to move
+ * existing backups to a different location.
+ */
+ private void validateRootPathCanBeOverridden(String originalPath, String
backupId)
+ throws IOException {
+ String anotherRootDir = "/some/other/root/dir";
+ Path anotherPath = new Path(anotherRootDir, backupId);
+ BackupManifest.BackupImage differentLocationImage =
BackupManifest.hydrateRootDir(
+ HBackupFileSystem.getManifest(conf1, new Path(originalPath),
backupId).getBackupImage(),
+ anotherPath);
+ assertEquals(differentLocationImage.getRootDir(), anotherRootDir);
+ for (BackupManifest.BackupImage ancestor :
differentLocationImage.getAncestors()) {
+ assertEquals(anotherRootDir, ancestor.getRootDir());
+ }
+ }
}
diff --git a/hbase-protocol-shaded/src/main/protobuf/Backup.proto
b/hbase-protocol-shaded/src/main/protobuf/Backup.proto
index afe43122f84..a114001ba50 100644
--- a/hbase-protocol-shaded/src/main/protobuf/Backup.proto
+++ b/hbase-protocol-shaded/src/main/protobuf/Backup.proto
@@ -54,11 +54,12 @@ message TableServerTimestamp {
/**
* Structure keeps relevant info for backup restore session
+ * backup_root_dir was marked as deprecated in HBase 2.6.2, will be removed in
4.0.0.
*/
message BackupImage {
optional string backup_id = 1;
optional BackupType backup_type = 2;
- optional string backup_root_dir = 3;
+ optional string backup_root_dir = 3 [deprecated = true];
repeated TableName table_list = 4;
optional uint64 start_ts = 5;
optional uint64 complete_ts = 6;
