Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
janvanbesien commented on code in PR #7761: URL: https://github.com/apache/hbase/pull/7761#discussion_r3143181722 ## hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java: ## Review Comment: I fixed all your remarks. However, when trying to rebase on master, I noticed that the test seems to work now without any code changes. So either the problem has been fixed it in the mean time via another change, or my test is no good. I pushed it like this (test only) but I should find some more time to deep dive. -- 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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
DieterDePaepe commented on PR #7761: URL: https://github.com/apache/hbase/pull/7761#issuecomment-4184591724 Force-pushed a rebase on current master. -- 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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
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 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 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 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 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();
+
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
DieterDePaepe commented on PR #7761: URL: https://github.com/apache/hbase/pull/7761#issuecomment-4185382776 @hgromer Could you also review this PR? -- 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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
DieterDP-ng commented on PR #7761: URL: https://github.com/apache/hbase/pull/7761#issuecomment-4184594524 I've force-pushed a rebase on the current master. -- 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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
janvanbesien commented on code in PR #7761:
URL: https://github.com/apache/hbase/pull/7761#discussion_r2817178187
##
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##
@@ -215,6 +216,59 @@ public void testBackupLogCleaner() throws Exception {
}
}
+ @Test
+ public void testRemovedTableDoesNotPinWals() throws Exception {
Review Comment:
Thanks for your review dieter.
As I understand it, the problem can be triggered by explicitly removing a
table from the backup (i.e. explicitly deciding to no longer backup said table)
or by removing the table completely (i.e. implicitly it is no longer in any
future backup either). You're right that I mixed the two cases and that the
primary trigger was the latter, not the former.
I now have a test for both scenarios. Does that make sense?
Also updated commit message and rebased on master.
--
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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
DieterDP-ng commented on code in PR #7761:
URL: https://github.com/apache/hbase/pull/7761#discussion_r2816338701
##
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##
@@ -215,6 +216,59 @@ public void testBackupLogCleaner() throws Exception {
}
}
+ @Test
+ public void testRemovedTableDoesNotPinWals() throws Exception {
Review Comment:
I suggest to also modify your commit message a bit to explain the main use
case being tables that have since been deleted.
--
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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
DieterDP-ng commented on code in PR #7761:
URL: https://github.com/apache/hbase/pull/7761#discussion_r2816319265
##
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java:
##
@@ -215,6 +216,59 @@ public void testBackupLogCleaner() throws Exception {
}
}
+ @Test
+ public void testRemovedTableDoesNotPinWals() throws Exception {
Review Comment:
The name of this test is misleading. I'd also add a textual description as
to the specific case you're checking here.
The current name implies it has to do with a table that was removed. That is
also the problem originally encountered (in NGDATA): logs were not being
removed for a table that has already been removed. But you're not testing for
that here, this test doesn't remove a table.
There's a difference in the logic that's relevant. If you were to extend
your testcase to the following: `full backup F1`, `incremental I1`, `full
backup F2` (table 1 only), `incremental I2` (table 1 only), `delete F1, I1`,
I'm pretty sure the test would fail because `I2` would still refer to table2.
However, if you deleted table 2 after `I1`, I suspect it will work as
expected.
--
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]
Re: [PR] HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set [hbase]
DieterDP-ng commented on PR #7761: URL: https://github.com/apache/hbase/pull/7761#issuecomment-3913836151 Hi Jan, 2 remarks regarding the format of the PR: - HBASE accepts PRs for the master branch. If accepted for master, it's backported to the other branches by the person doing the merge. - HBASE asks that the commit header message matches the title of the ticket it solves. -- 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]
