[GitHub] [hbase] Apache9 commented on a change in pull request #3359: HBASE-25891 remove dependence storing wal filenames for backup

[GitBox <gi...@apache.org>]

Apache9 commented on a change in pull request #3359:
URL: https://github.com/apache/hbase/pull/3359#discussion_r677913764



##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupInfo.java
##########
@@ -351,19 +355,19 @@ public void setIncrBackupFileList(List<String> incrBackupFileList) {
 
   /**
    * Set the new region server log timestamps after distributed log roll
-   * @param newTableSetTimestampMap table timestamp map
+   * @param prevTableSetTimestampMap table timestamp map
    */
-  public void setIncrTimestampMap(HashMap<TableName,
-          HashMap<String, Long>> newTableSetTimestampMap) {
-    this.tableSetTimestampMap = newTableSetTimestampMap;
+  public void setIncrTimestampMap(Map<TableName,
+          Map<String, Long>> prevTableSetTimestampMap) {
+    this.incrTimestampMap = prevTableSetTimestampMap;

Review comment:
       Is this a bug in the old code? We do not change the name of the method but change the implementation?

##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java
##########
@@ -99,68 +95,19 @@ public IncrementalBackupManager(Connection conn, Configuration conf) throws IOEx
     newTimestamps = readRegionServerLastLogRollResult();
 
     logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
-    List<WALItem> logFromSystemTable =
-        getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps, getBackupInfo()
-            .getBackupRootDir());
-    logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
+    logList = excludeProcV2WALs(logList);
     backupInfo.setIncrBackupFileList(logList);
 
     return newTimestamps;
   }
 
-  /**
-   * Get list of WAL files eligible for incremental backup.
-   *
-   * @return list of WAL files
-   * @throws IOException if getting the list of WAL files fails
-   */
-  public List<String> getIncrBackupLogFileList() throws IOException {
-    List<String> logList;
-    HashMap<String, Long> newTimestamps;
-    HashMap<String, Long> previousTimestampMins;
-
-    String savedStartCode = readBackupStartCode();
-
-    // key: tableName
-    // value: <RegionServer,PreviousTimeStamp>
-    HashMap<TableName, HashMap<String, Long>> previousTimestampMap = readLogTimestampMap();
-
-    previousTimestampMins = BackupUtils.getRSLogTimestampMins(previousTimestampMap);
-
-    if (LOG.isDebugEnabled()) {
-      LOG.debug("StartCode " + savedStartCode + "for backupID " + backupInfo.getBackupId());
-    }
-    // get all new log files from .logs and .oldlogs after last TS and before new timestamp
-    if (savedStartCode == null || previousTimestampMins == null
-        || previousTimestampMins.isEmpty()) {
-      throw new IOException(
-          "Cannot read any previous back up timestamps from backup system table. "
-              + "In order to create an incremental backup, at least one full backup is needed.");
-    }
-
-    newTimestamps = readRegionServerLastLogRollResult();
-
-    logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps, conf, savedStartCode);
-    List<WALItem> logFromSystemTable =
-        getLogFilesFromBackupSystem(previousTimestampMins, newTimestamps, getBackupInfo()
-            .getBackupRootDir());
-
-    logList = excludeAlreadyBackedUpAndProcV2WALs(logList, logFromSystemTable);
-    backupInfo.setIncrBackupFileList(logList);
-
-    return logList;
-  }
-
-  private List<String> excludeAlreadyBackedUpAndProcV2WALs(List<String> logList,
-      List<WALItem> logFromSystemTable) {
-    Set<String> walFileNameSet = convertToSet(logFromSystemTable);
-
+  private List<String> excludeProcV2WALs(List<String> logList) {
     List<String> list = new ArrayList<>();
     for (int i=0; i < logList.size(); i++) {
       Path p = new Path(logList.get(i));
       String name  = p.getName();
 
-      if (walFileNameSet.contains(name) || name.startsWith(WALProcedureStore.LOG_PREFIX)) {
+      if (name.startsWith(WALProcedureStore.LOG_PREFIX)) {

Review comment:
       On master branch we do not have WALProcedure any more. Now we use RegionProcedureStore, it has its own WAL. can file another issue to review this.

##########
File path: hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
##########
@@ -178,11 +174,10 @@ public String toString() {
   final static byte[] BL_PREPARE = Bytes.toBytes("R");

Review comment:
       nits: Open another PR to change the style, it should be 'static final' instead of 'final static'. This could reduce the checkstyle warnings.




-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org