ndimiduk commented on code in PR #5868:
URL: https://github.com/apache/hbase/pull/5868#discussion_r1609532907
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java:
##########
@@ -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<>();
+ Set<TableName> tablesToCover = new HashSet<>(backupInfo.getTables());
+
+ // Go over the backup history list in descending order (newest to oldest)
+ List<BackupInfo> allHistoryList = backupManager.getBackupHistory(true);
Review Comment:
Should this `getBackupHistory()` method support filters? Could we reduce the
search space for applicable backups before entering this loop?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java:
##########
@@ -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<>();
Review Comment:
Should the ancestors be a `Set` so that we don't end up with multiple
entries for the same ancestor?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java:
##########
@@ -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<>();
+ Set<TableName> tablesToCover = new HashSet<>(backupInfo.getTables());
+
+ // Go over the backup history list in descending order (newest to oldest)
+ List<BackupInfo> 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())) {
Review Comment:
nit: is there a case where either side of this comparison can be null (even
in a mocked-out unit test)? Better to use `Objects.equals()` instead?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java:
##########
@@ -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<>();
+ Set<TableName> tablesToCover = new HashSet<>(backupInfo.getTables());
+
+ // Go over the backup history list in descending order (newest to oldest)
+ List<BackupInfo> 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;
Review Comment:
I think you can fix the static analysis warning by wrapping this return in a
`Collections.immutableList()`.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/TableBackupClient.java:
##########
@@ -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<>();
+ Set<TableName> tablesToCover = new HashSet<>(backupInfo.getTables());
+
+ // Go over the backup history list in descending order (newest to oldest)
Review Comment:
Thank you for clarifying what "descending order" means. In fact, you can
strike "descending order" entirely and replace it with your clarifying
description.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java:
##########
@@ -295,51 +298,31 @@ public ArrayList<BackupImage> 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 the image has a different rootDir, it cannot be an ancestor.
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);
- }
+ // 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)) {
Review Comment:
@rmdmattingly do we need to add your new unit test someplace?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
