ndimiduk commented on code in PR #6040:
URL: https://github.com/apache/hbase/pull/6040#discussion_r1766371119
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -71,44 +77,43 @@ public void testBackupLogCleaner() throws Exception {
// Verify that we have no backup sessions yet
assertFalse(systemTable.hasBackupSessions());
- List<FileStatus> walFiles =
getListOfWALFiles(TEST_UTIL.getConfiguration());
+ List<FileStatus> walFilesBeforeBackup =
getListOfWALFiles(TEST_UTIL.getConfiguration());
BackupLogCleaner cleaner = new BackupLogCleaner();
cleaner.setConf(TEST_UTIL.getConfiguration());
Map<String, Object> params = new HashMap<>();
params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
cleaner.init(params);
cleaner.setConf(TEST_UTIL.getConfiguration());
- Iterable<FileStatus> deletable = cleaner.getDeletableFiles(walFiles);
- int size = Iterables.size(deletable);
-
// We can delete all files because we do not have yet recorded backup
sessions
- assertTrue(size == walFiles.size());
+ Iterable<FileStatus> deletable =
cleaner.getDeletableFiles(walFilesBeforeBackup);
+ int size = Iterables.size(deletable);
+ assertEquals(walFilesBeforeBackup.size(), size);
- String backupIdFull = fullTableBackup(tableSetFullList);
+ // Create a FULL backup (backupRoot 1)
+ String backupIdFull = backupTables(BackupType.FULL, tableSetFullList,
backupRoot1.toString());
assertTrue(checkSucceeded(backupIdFull));
- // Check one more time
- deletable = cleaner.getDeletableFiles(walFiles);
- // We can delete wal files because they were saved into backup system
table table
- size = Iterables.size(deletable);
- assertTrue(size == walFiles.size());
- List<FileStatus> newWalFiles =
getListOfWALFiles(TEST_UTIL.getConfiguration());
- LOG.debug("WAL list after full backup");
+ // New list of WAL files is greater than the previous one,
+ // because new WAL per RS have been opened after full backup
+ Set<FileStatus> walFilesAfterFullBackup =
+ mergeAsSet(walFilesBeforeBackup,
getListOfWALFiles(TEST_UTIL.getConfiguration()));
+ assertTrue(walFilesBeforeBackup.size() < walFilesAfterFullBackup.size());
- // New list of wal files is greater than the previous one,
- // because new wal per RS have been opened after full backup
- assertTrue(walFiles.size() < newWalFiles.size());
+ // We can only delete the WALs preceding the FULL backup
+ deletable = cleaner.getDeletableFiles(walFilesAfterFullBackup);
+ size = Iterables.size(deletable);
+ assertEquals(walFilesBeforeBackup.size(), size);
+
+ // Insert some data
Connection conn = ConnectionFactory.createConnection(conf1);
- // #2 - insert some data to table
Table t1 = conn.getTable(table1);
Review Comment:
nit: please wrap this in a try-with-resource block.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -71,44 +77,43 @@ public void testBackupLogCleaner() throws Exception {
// Verify that we have no backup sessions yet
assertFalse(systemTable.hasBackupSessions());
- List<FileStatus> walFiles =
getListOfWALFiles(TEST_UTIL.getConfiguration());
+ List<FileStatus> walFilesBeforeBackup =
getListOfWALFiles(TEST_UTIL.getConfiguration());
BackupLogCleaner cleaner = new BackupLogCleaner();
cleaner.setConf(TEST_UTIL.getConfiguration());
Map<String, Object> params = new HashMap<>();
params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
cleaner.init(params);
cleaner.setConf(TEST_UTIL.getConfiguration());
- Iterable<FileStatus> deletable = cleaner.getDeletableFiles(walFiles);
- int size = Iterables.size(deletable);
-
// We can delete all files because we do not have yet recorded backup
sessions
- assertTrue(size == walFiles.size());
+ Iterable<FileStatus> deletable =
cleaner.getDeletableFiles(walFilesBeforeBackup);
+ int size = Iterables.size(deletable);
+ assertEquals(walFilesBeforeBackup.size(), size);
- String backupIdFull = fullTableBackup(tableSetFullList);
+ // Create a FULL backup (backupRoot 1)
+ String backupIdFull = backupTables(BackupType.FULL, tableSetFullList,
backupRoot1.toString());
assertTrue(checkSucceeded(backupIdFull));
- // Check one more time
- deletable = cleaner.getDeletableFiles(walFiles);
- // We can delete wal files because they were saved into backup system
table table
- size = Iterables.size(deletable);
- assertTrue(size == walFiles.size());
- List<FileStatus> newWalFiles =
getListOfWALFiles(TEST_UTIL.getConfiguration());
- LOG.debug("WAL list after full backup");
+ // New list of WAL files is greater than the previous one,
+ // because new WAL per RS have been opened after full backup
+ Set<FileStatus> walFilesAfterFullBackup =
+ mergeAsSet(walFilesBeforeBackup,
getListOfWALFiles(TEST_UTIL.getConfiguration()));
+ assertTrue(walFilesBeforeBackup.size() < walFilesAfterFullBackup.size());
- // New list of wal files is greater than the previous one,
- // because new wal per RS have been opened after full backup
- assertTrue(walFiles.size() < newWalFiles.size());
+ // We can only delete the WALs preceding the FULL backup
+ deletable = cleaner.getDeletableFiles(walFilesAfterFullBackup);
+ size = Iterables.size(deletable);
+ assertEquals(walFilesBeforeBackup.size(), size);
+
+ // Insert some data
Connection conn = ConnectionFactory.createConnection(conf1);
Review Comment:
nit: this should use the connection managed by `TEST_UTIL`.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -118,23 +123,49 @@ public void testBackupLogCleaner() throws Exception {
p2.addColumn(famName, qualName, Bytes.toBytes("val" + i));
t2.put(p2);
}
-
t2.close();
- // #3 - incremental backup for multiple tables
-
+ // Create an INCREMENTAL backup (backupRoot 1)
List<TableName> tableSetIncList = Lists.newArrayList(table1, table2,
table3);
String backupIdIncMultiple =
- backupTables(BackupType.INCREMENTAL, tableSetIncList, BACKUP_ROOT_DIR);
+ backupTables(BackupType.INCREMENTAL, tableSetIncList,
backupRoot1.toString());
assertTrue(checkSucceeded(backupIdIncMultiple));
- deletable = cleaner.getDeletableFiles(newWalFiles);
- assertTrue(Iterables.size(deletable) == newWalFiles.size());
+ // There should be more WALs due to the rolling of Region Servers
+ Set<FileStatus> walFilesAfterIncBackup =
+ mergeAsSet(walFilesAfterFullBackup,
getListOfWALFiles(TEST_UTIL.getConfiguration()));
+ assertTrue(walFilesAfterFullBackup.size() <
walFilesAfterIncBackup.size());
+
+ // We can only delete the WALs preceding the INCREMENTAL backup
+ deletable = cleaner.getDeletableFiles(walFilesAfterIncBackup);
+ size = Iterables.size(deletable);
+ assertEquals(walFilesAfterFullBackup.size(), size);
+
+ // Create a FULL backup (backupRoot 2)
+ String backupIdFull2 = backupTables(BackupType.FULL, tableSetIncList,
backupRoot2.toString());
+ assertTrue(checkSucceeded(backupIdFull2));
+
+ // There should be more WALs due to the rolling of Region Servers
+ Set<FileStatus> walFilesAfterFullBackup2 =
+ mergeAsSet(walFilesAfterFullBackup,
getListOfWALFiles(TEST_UTIL.getConfiguration()));
+ assertTrue(walFilesAfterIncBackup.size() <
walFilesAfterFullBackup2.size());
+
+ // We created a backup in a different root, so the WAL dependencies of
the first root did not
+ // change. I.e. the same files should be deletable as after the
incremental backup.
Review Comment:
Should we check this condition more strictly -- equality on the set of
paths, instead of just checking set cardinality?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -81,39 +81,55 @@ public void init(Map<String, Object> params) {
}
}
- private Map<Address, Long> getServerToNewestBackupTs(List<BackupInfo>
backups)
+ /**
+ * Calculates the timestamp boundary up to which all backup roots have
already included the WAL.
+ * I.e. WALs with a lower (= older) or equal timestamp are no longer needed
for future incremental
+ * backups.
+ */
+ private Map<Address, Long> serverToPreservationBoundaryTs(List<BackupInfo>
backups)
Review Comment:
Thank you for the explanation, both this and below. I think I follow you.
Let me rephrase.
`newestBackupPerRootDir` is a "race to the top" in terms of identifying the
most recent timestamp across backups, while `boundaries` is a "race to the
bottom" in terms of identifying the least recent backup present on each server.
By selecting the "oldest of the newest" you determine the minimum timestamp
that must be preserved for each server. Any WALs older than this minimum
timestamp can be safely discarded.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -61,8 +65,10 @@ public class TestBackupLogCleaner extends TestBackupBase {
@Test
public void testBackupLogCleaner() throws Exception {
Review Comment:
This test is not quite what you described in your earlier comment, and I'm
pretty sure that it doesn't test the criteria in the same way either. It's a
valid test case, but I think it's missing some cases that we would like to see
covered.
In this test, we have a single region server, which serves as S2 from your
above example.
1. At T0 you take a full backup B1 of all four tables at R1. `boundary` for
our server is the timestamp of oldest of the wals of all the tables.
1. Data is inserted into tables 1 and 2.
1. At T1 you take incremental backup B1 of tables 1, 2, 3 to R1. `boundary`
for our server doesn't change because the oldest wal is still from B1 and table
4.
1. At T2 you take a full backup B2 of tables 1, 2, 3 to R2. `boundary` does
not change because the oldest wal is still from B1 and table 4.
Based on my understanding, this test never sees a point when a wal can be
discarded because wals from table4 are never rolled and so the wals identified
for table4 in B1 are always the "oldest of the newest" for this server.
I think that we'd like to see a test that does what you described in your
initial comment: observes the "rising tide" of wal timestamp on the server as
backups across roots progress.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##########
@@ -71,44 +77,43 @@ public void testBackupLogCleaner() throws Exception {
// Verify that we have no backup sessions yet
assertFalse(systemTable.hasBackupSessions());
- List<FileStatus> walFiles =
getListOfWALFiles(TEST_UTIL.getConfiguration());
+ List<FileStatus> walFilesBeforeBackup =
getListOfWALFiles(TEST_UTIL.getConfiguration());
BackupLogCleaner cleaner = new BackupLogCleaner();
cleaner.setConf(TEST_UTIL.getConfiguration());
Map<String, Object> params = new HashMap<>();
params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
cleaner.init(params);
cleaner.setConf(TEST_UTIL.getConfiguration());
- Iterable<FileStatus> deletable = cleaner.getDeletableFiles(walFiles);
- int size = Iterables.size(deletable);
-
// We can delete all files because we do not have yet recorded backup
sessions
- assertTrue(size == walFiles.size());
+ Iterable<FileStatus> deletable =
cleaner.getDeletableFiles(walFilesBeforeBackup);
+ int size = Iterables.size(deletable);
+ assertEquals(walFilesBeforeBackup.size(), size);
- String backupIdFull = fullTableBackup(tableSetFullList);
+ // Create a FULL backup (backupRoot 1)
+ String backupIdFull = backupTables(BackupType.FULL, tableSetFullList,
backupRoot1.toString());
assertTrue(checkSucceeded(backupIdFull));
- // Check one more time
- deletable = cleaner.getDeletableFiles(walFiles);
- // We can delete wal files because they were saved into backup system
table table
- size = Iterables.size(deletable);
- assertTrue(size == walFiles.size());
- List<FileStatus> newWalFiles =
getListOfWALFiles(TEST_UTIL.getConfiguration());
- LOG.debug("WAL list after full backup");
+ // New list of WAL files is greater than the previous one,
+ // because new WAL per RS have been opened after full backup
+ Set<FileStatus> walFilesAfterFullBackup =
+ mergeAsSet(walFilesBeforeBackup,
getListOfWALFiles(TEST_UTIL.getConfiguration()));
+ assertTrue(walFilesBeforeBackup.size() < walFilesAfterFullBackup.size());
- // New list of wal files is greater than the previous one,
- // because new wal per RS have been opened after full backup
- assertTrue(walFiles.size() < newWalFiles.size());
+ // We can only delete the WALs preceding the FULL backup
+ deletable = cleaner.getDeletableFiles(walFilesAfterFullBackup);
+ size = Iterables.size(deletable);
+ assertEquals(walFilesBeforeBackup.size(), size);
+
+ // Insert some data
Connection conn = ConnectionFactory.createConnection(conf1);
- // #2 - insert some data to table
Table t1 = conn.getTable(table1);
Put p1;
for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
p1 = new Put(Bytes.toBytes("row-t1" + i));
p1.addColumn(famName, qualName, Bytes.toBytes("val" + i));
t1.put(p1);
}
-
t1.close();
Table t2 = conn.getTable(table2);
Review Comment:
try-with-resource here as well please.
--
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]
