rmdmattingly commented on code in PR #6134:
URL: https://github.com/apache/hbase/pull/6134#discussion_r1754942675
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -792,20 +755,19 @@ public List<BackupInfo> getBackupHistory(int n,
BackupInfo.Filter... filters) th
return result;
}
- /*
- * Retrieve TableName's for completed backup of given type
- * @param type backup type
- * @return List of table names
+ /**
+ * Retrieve all table names that are part of any known backup
*/
- public List<TableName> getTablesForBackupType(BackupType type) throws
IOException {
+ public Set<TableName> getTablesForBackupType() throws IOException {
Review Comment:
Yeah, getFullyBackedUpTables or getTablesWithFullBackups would be improve
the clarity
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -400,43 +395,22 @@ public void deleteBackupInfo(String backupId) throws
IOException {
}
}
- /*
- * For postBulkLoadHFile() hook.
- * @param tabName table name
- * @param region the region receiving hfile
- * @param finalPaths family and associated hfiles
+ /**
+ * Registers a bulk load.
+ * @param tabName table name
+ * @param region the region receiving hfile
+ * @param cfToHfilePath column family and associated hfiles
*/
- public void writePathsPostBulkLoad(TableName tabName, byte[] region,
- Map<byte[], List<Path>> finalPaths) throws IOException {
+ public void registerBulkLoad(TableName tabName, byte[] region,
+ Map<byte[], List<Path>> cfToHfilePath) throws IOException {
if (LOG.isDebugEnabled()) {
- LOG.debug("write bulk load descriptor to backup " + tabName + " with " +
finalPaths.size()
- + " entries");
+ LOG.debug("Writing bulk load descriptor to backup {} with {} entries",
tabName,
+ cfToHfilePath.size());
}
try (Table table = connection.getTable(bulkLoadTableName)) {
- List<Put> puts =
BackupSystemTable.createPutForCommittedBulkload(tabName, region, finalPaths);
+ List<Put> puts = BackupSystemTable.createPutForBulkLoad(tabName, region,
cfToHfilePath);
Review Comment:
While we're here, it's probably worth using a BufferedMutator like we did in
https://github.com/apache/hbase/pull/6067
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -1579,30 +1521,8 @@ public static void deleteSnapshot(Connection conn)
throws IOException {
}
}
- /*
- * Creates Put's for bulk load resulting from running LoadIncrementalHFiles
- */
- static List<Put> createPutForPreparedBulkload(TableName table, byte[]
region, final byte[] family,
- final List<Pair<Path, Path>> pairs) {
- List<Put> puts = new ArrayList<>(pairs.size());
- for (Pair<Path, Path> pair : pairs) {
- Path path = pair.getSecond();
- String file = path.toString();
- int lastSlash = file.lastIndexOf("/");
- String filename = file.substring(lastSlash + 1);
- Put put = new Put(rowkey(BULK_LOAD_PREFIX, table.toString(),
BLK_LD_DELIM,
- Bytes.toString(region), BLK_LD_DELIM, filename));
- put.addColumn(BackupSystemTable.META_FAMILY, TBL_COL, table.getName());
- put.addColumn(BackupSystemTable.META_FAMILY, FAM_COL, family);
- put.addColumn(BackupSystemTable.META_FAMILY, PATH_COL,
Bytes.toBytes(file));
- put.addColumn(BackupSystemTable.META_FAMILY, STATE_COL, BL_PREPARE);
- puts.add(put);
- LOG.debug("writing raw bulk path " + file + " for " + table + " " +
Bytes.toString(region));
- }
- return puts;
- }
-
public static List<Delete> createDeleteForOrigBulkLoad(List<TableName> lst) {
+ // Unused???
Review Comment:
should we act on this comment now?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -400,43 +395,22 @@ public void deleteBackupInfo(String backupId) throws
IOException {
}
}
- /*
- * For postBulkLoadHFile() hook.
- * @param tabName table name
- * @param region the region receiving hfile
- * @param finalPaths family and associated hfiles
+ /**
+ * Registers a bulk load.
+ * @param tabName table name
+ * @param region the region receiving hfile
+ * @param cfToHfilePath column family and associated hfiles
*/
- public void writePathsPostBulkLoad(TableName tabName, byte[] region,
- Map<byte[], List<Path>> finalPaths) throws IOException {
+ public void registerBulkLoad(TableName tabName, byte[] region,
+ Map<byte[], List<Path>> cfToHfilePath) throws IOException {
if (LOG.isDebugEnabled()) {
- LOG.debug("write bulk load descriptor to backup " + tabName + " with " +
finalPaths.size()
- + " entries");
+ LOG.debug("Writing bulk load descriptor to backup {} with {} entries",
tabName,
+ cfToHfilePath.size());
}
try (Table table = connection.getTable(bulkLoadTableName)) {
- List<Put> puts =
BackupSystemTable.createPutForCommittedBulkload(tabName, region, finalPaths);
+ List<Put> puts = BackupSystemTable.createPutForBulkLoad(tabName, region,
cfToHfilePath);
table.put(puts);
- LOG.debug("written " + puts.size() + " rows for bulk load of " +
tabName);
- }
- }
-
- /*
- * For preCommitStoreFile() hook
- * @param tabName table name
- * @param region the region receiving hfile
- * @param family column family
- * @param pairs list of paths for hfiles
- */
- public void writeFilesForBulkLoadPreCommit(TableName tabName, byte[] region,
final byte[] family,
- final List<Pair<Path, Path>> pairs) throws IOException {
- if (LOG.isDebugEnabled()) {
- LOG.debug(
- "write bulk load descriptor to backup " + tabName + " with " +
pairs.size() + " entries");
- }
- try (Table table = connection.getTable(bulkLoadTableName)) {
- List<Put> puts =
- BackupSystemTable.createPutForPreparedBulkload(tabName, region,
family, pairs);
- table.put(puts);
- LOG.debug("written " + puts.size() + " rows for bulk load of " +
tabName);
+ LOG.debug("Written {} rows for bulk load of {}", puts.size(), tabName);
Review Comment:
while we're here, can we change `tabName` to `tableName`?
--
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]
