ndimiduk commented on code in PR #6533:
URL: https://github.com/apache/hbase/pull/6533#discussion_r1905628516
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -1703,7 +1633,7 @@ static String getRegionNameFromOrigBulkLoadRow(String
rowStr) {
}
/*
- * Used to query bulk loaded hfiles which have been copied by incremental
backup
+ * Do not use - see HBASE-28715
Review Comment:
Err, what? Can de deprecate/delete this method instead of this comment?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupHFileCleaner.java:
##########
@@ -18,19 +18,18 @@
package org.apache.hadoop.hbase.backup;
import java.io.IOException;
-import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
import java.util.Set;
+import org.apache.commons.lang3.NotImplementedException;
Review Comment:
TIL: commons-lang3 is not a shaded dependency, which means it's part of our
public api :/
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupHFileCleaner.java:
##########
@@ -94,55 +92,47 @@ public void cleanup() {
@Test
public void testGetDeletableFiles() throws IOException {
- // 1. Create a file
- Path file = new Path(root, "testIsFileDeletableWithNoHFileRefs");
- fs.createNewFile(file);
- // 2. Assert file is successfully created
- assertTrue("Test file not created!", fs.exists(file));
- BackupHFileCleaner cleaner = new BackupHFileCleaner();
- cleaner.setConf(conf);
- cleaner.setCheckForFullyBackedUpTables(false);
- List<FileStatus> stats = new ArrayList<>();
- // Prime the cleaner
- cleaner.getDeletableFiles(stats);
- // 3. Assert that file as is should be deletable
- FileStatus stat = fs.getFileStatus(file);
- stats.add(stat);
- Iterable<FileStatus> deletable = cleaner.getDeletableFiles(stats);
- boolean found = false;
- for (FileStatus stat1 : deletable) {
- if (stat.equals(stat1)) {
- found = true;
- }
- }
- assertTrue(
- "Cleaner should allow to delete this file as there is no hfile reference
" + "for it.",
- found);
-
- // 4. Add the file as bulk load
- List<Path> list = new ArrayList<>(1);
- list.add(file);
- try (Connection conn = ConnectionFactory.createConnection(conf);
- BackupSystemTable sysTbl = new BackupSystemTable(conn)) {
- List<TableName> sTableList = new ArrayList<>();
- sTableList.add(tableName);
- @SuppressWarnings("unchecked")
- IdentityHashMap<byte[], List<Path>>[] maps = new IdentityHashMap[1];
- maps[0] = new IdentityHashMap<>();
- maps[0].put(Bytes.toBytes(famName), list);
- sysTbl.writeBulkLoadedFiles(sTableList, maps, "1");
- }
+ FileStatus file1 = createFile("file1");
+ FileStatus file1Archived = createFile("archived/file1");
+ FileStatus file2 = createFile("file2");
+ FileStatus file3 = createFile("file3");
- // 5. Assert file should not be deletable
- deletable = cleaner.getDeletableFiles(stats);
- found = false;
- for (FileStatus stat1 : deletable) {
- if (stat.equals(stat1)) {
- found = true;
+ BackupHFileCleaner cleaner = new BackupHFileCleaner() {
+ @Override
+ protected Set<TableName> fetchFullyBackedUpTables(BackupSystemTable tbl)
{
+ return Set.of(tableNameWithBackup);
}
+ };
+ cleaner.setConf(conf);
+
+ Iterable<FileStatus> deletable;
+
+ // The first call will not allow any deletions because of the timestamp
mechanism.
+ deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2,
file3));
+ assertEquals(List.of(), Lists.newArrayList(deletable));
+
+ // No bulk loads registered, so all files can be deleted.
+ deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2,
file3));
+ assertEquals(List.of(file1, file1Archived, file2, file3),
Lists.newArrayList(deletable));
Review Comment:
nit: Should these assertions be more flexible wrt entry order?
--
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]
