DieterDePaepe commented on code in PR #7761:
URL: https://github.com/apache/hbase/pull/7761#discussion_r3034496176
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -205,6 +208,154 @@ public void testBackupLogCleaner() throws Exception {
}
}
+ /**
+ * Verify that when a table is no longer in the backup set, it doesn't block
WAL cleanup.
+ */
+ @Test
+ public void testRemovedBackupDoesNotPinWals() throws Exception {
+ Path backupRoot = new Path(BACKUP_ROOT_DIR, "staleRoot");
+
+ try {
+ BackupLogCleaner cleaner = new BackupLogCleaner();
+ cleaner.setConf(TEST_UTIL.getConfiguration());
+ Map<String, Object> params = new HashMap<>(1);
+ params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
+ cleaner.init(params);
+
+ // Create FULL backup B1 with table1 and table2
+ String backupIdB1 =
+ backupTables(BackupType.FULL, Arrays.asList(table1, table2),
backupRoot.toString());
+ assertTrue(checkSucceeded(backupIdB1));
+
+ Set<FileStatus> walFilesAfterB1 =
+ new LinkedHashSet<>(getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+ // Insert data so the next backup advances WAL positions for table1
+ Connection conn = TEST_UTIL.getConnection();
+ try (Table t1 = conn.getTable(table1)) {
+ for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
+ Put p = new Put(Bytes.toBytes("stale-row-t1" + i));
+ p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+ t1.put(p);
+ }
+ }
+
+ // Create FULL backup B2 with only table1.
+ // B2's tableSetTimestampMap carries forward the old timestamp from B1
for table2,
+ // while table1 gets a fresh timestamp: { table1: ts(B2), table2: ts(B1)
}
+ String backupIdB2 =
+ backupTables(BackupType.FULL, Collections.singletonList(table1),
backupRoot.toString());
+ assertTrue(checkSucceeded(backupIdB2));
+
+ Set<FileStatus> walFilesAfterB2 =
+ mergeAsSet(walFilesAfterB1,
getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+ // Delete B1: since it is the only backup referencing table2,
finalizeDelete will
+ // remove table2 from the incremental backup set for this root.
+ getBackupAdmin().deleteBackups(new String[] { backupIdB1 });
+
+ // table2 is no longer in the backup set, so the boundary = ts(B2)
instead of
+ // min(ts(B2), ts(B1)) = ts(B1). WALs between B1 and B2 are now
deletable.
+ Iterable<FileStatus> deletable =
cleaner.getDeletableFiles(walFilesAfterB2);
+ assertTrue(toSet(deletable).containsAll(walFilesAfterB1),
+ "WALs after B1 should be deletable once stale tables are removed from
incr set");
+ } finally {
+
TEST_UTIL.truncateTable(BackupSystemTable.getTableName(TEST_UTIL.getConfiguration())).close();
+ }
+ }
+
+ /**
+ * Similar as {@link #testRemovedBackupDoesNotPinWals()} but for the case
where a table is
+ * removed, and hence no longer in any backup either.
+ */
+ @Test
+ public void testRemovedTableDoesNotPinWals() throws Exception {
Review Comment:
You probably added this test due to my previous comment, but seeing it in
action now, I now realize it serves no purpose. The fact that the table is
removed has no effect (i.e. all assertions are fine even if the table isn't
removed).
The mechanism added in this PR is that the logcleaner only cares about the
tables still in scope for backups, and that's covered in the first test.
Given that this test takes +-30secs to run, I think its best to remove it.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
Two observations that I had while reviewing, but not something that needs
fixing in this PR. Mainly thinking out loud, and might log issues for these.
- If a table is deleted, it will result in WAL files sticking around until
all backups containing that deleted table are gone (because that is when the
incremental backup set gets updated). If long backup retention times are used,
this can still be problematic. There's room for further improvement here.
- If a table is deleted and recreated with the same name, the backup info
will still have the timestamps of the deleted table, and WALs that are included
in incremental backups will also contain data of the deleted table. This would
lead to a corrupted restore.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -205,6 +208,154 @@ public void testBackupLogCleaner() throws Exception {
}
}
+ /**
+ * Verify that when a table is no longer in the backup set, it doesn't block
WAL cleanup.
+ */
+ @Test
+ public void testRemovedBackupDoesNotPinWals() throws Exception {
+ Path backupRoot = new Path(BACKUP_ROOT_DIR, "staleRoot");
+
+ try {
+ BackupLogCleaner cleaner = new BackupLogCleaner();
+ cleaner.setConf(TEST_UTIL.getConfiguration());
+ Map<String, Object> params = new HashMap<>(1);
+ params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
+ cleaner.init(params);
+
+ // Create FULL backup B1 with table1 and table2
+ String backupIdB1 =
+ backupTables(BackupType.FULL, Arrays.asList(table1, table2),
backupRoot.toString());
+ assertTrue(checkSucceeded(backupIdB1));
+
+ Set<FileStatus> walFilesAfterB1 =
+ new LinkedHashSet<>(getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+ // Insert data so the next backup advances WAL positions for table1
+ Connection conn = TEST_UTIL.getConnection();
+ try (Table t1 = conn.getTable(table1)) {
+ for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
+ Put p = new Put(Bytes.toBytes("stale-row-t1" + i));
+ p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+ t1.put(p);
+ }
+ }
+
+ // Create FULL backup B2 with only table1.
+ // B2's tableSetTimestampMap carries forward the old timestamp from B1
for table2,
+ // while table1 gets a fresh timestamp: { table1: ts(B2), table2: ts(B1)
}
Review Comment:
Would be useful to verify this in the test as well.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -205,6 +208,154 @@ public void testBackupLogCleaner() throws Exception {
}
}
+ /**
+ * Verify that when a table is no longer in the backup set, it doesn't block
WAL cleanup.
+ */
+ @Test
+ public void testRemovedBackupDoesNotPinWals() throws Exception {
+ Path backupRoot = new Path(BACKUP_ROOT_DIR, "staleRoot");
+
+ try {
+ BackupLogCleaner cleaner = new BackupLogCleaner();
+ cleaner.setConf(TEST_UTIL.getConfiguration());
+ Map<String, Object> params = new HashMap<>(1);
+ params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
+ cleaner.init(params);
+
+ // Create FULL backup B1 with table1 and table2
+ String backupIdB1 =
+ backupTables(BackupType.FULL, Arrays.asList(table1, table2),
backupRoot.toString());
+ assertTrue(checkSucceeded(backupIdB1));
+
+ Set<FileStatus> walFilesAfterB1 =
Review Comment:
Add a sanity check that this isn't empty (and for `walFilesAfterB2`).
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
Review Comment:
I rebased your changes to the latest master, and there was some refactoring
involved. Would appreciate a second pair of eyes to take a look at the new code.
--
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]
