Re: [PR] HBASE-29240: Backup ancestry trees should have the ability to be invalidated [hbase]

["DieterDP-ng (via GitHub)" <gi...@apache.org>]

DieterDP-ng commented on code in PR #6891:
URL: https://github.com/apache/hbase/pull/6891#discussion_r2079598772


##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupClientFactory.java:
##########
@@ -49,8 +58,45 @@ public static TableBackupClient create(Connection conn, String backupId, BackupR
     BackupType type = request.getBackupType();
     if (type == BackupType.FULL) {
       return new FullTableBackupClient(conn, backupId, request);
-    } else {
-      return new IncrementalTableBackupClient(conn, backupId, request);
     }
+
+    String latestFullBackup = getLatestFullBackupId(conn, request);
+
+    try (BackupAdmin admin = new BackupAdminImpl(conn)) {
+      boolean disallowFurtherIncrementals =
+        admin.getBackupInfo(latestFullBackup).isDisallowFurtherIncrementals();
+
+      if (!disallowFurtherIncrementals) {
+        return new IncrementalTableBackupClient(conn, backupId, request);
+      }
+
+      if (request.getFailOnDisallowedIncrementals()) {
+        throw new IncrementalBackupsDisallowedException(request);
+      }
+
+      LOG.info("Incremental backups disallowed for backupId {}, creating a full backup",

Review Comment:
   Nit: it might be unclear what the `backupId` refers to.



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupClientFactory.java:
##########
@@ -49,8 +58,45 @@ public static TableBackupClient create(Connection conn, String backupId, BackupR
     BackupType type = request.getBackupType();
     if (type == BackupType.FULL) {
       return new FullTableBackupClient(conn, backupId, request);
-    } else {
-      return new IncrementalTableBackupClient(conn, backupId, request);
     }
+
+    String latestFullBackup = getLatestFullBackupId(conn, request);
+
+    try (BackupAdmin admin = new BackupAdminImpl(conn)) {
+      boolean disallowFurtherIncrementals =
+        admin.getBackupInfo(latestFullBackup).isDisallowFurtherIncrementals();
+
+      if (!disallowFurtherIncrementals) {
+        return new IncrementalTableBackupClient(conn, backupId, request);
+      }
+
+      if (request.getFailOnDisallowedIncrementals()) {
+        throw new IncrementalBackupsDisallowedException(request);
+      }
+
+      LOG.info("Incremental backups disallowed for backupId {}, creating a full backup",
+        latestFullBackup);
+      return new FullTableBackupClient(conn, backupId, request);
+    }
+  }
+
+  private static String getLatestFullBackupId(Connection conn, BackupRequest request)

Review Comment:
   The log line above made me realize this approach is wrong.
   
   An incremental backup might build on top of multiple FULL backups: it includes all tables for which a FULL backup exists in the backup root. For example, given tables T1, T2, T3:
   - Timestamp 0: Full backup: T1, T2
   - Timestamp 1: Full backup: T1, T3
   - Timestamp 2: Incr backup: T1, T2, T3
   
   If we were to truncate T2, the full backup at time 0 would get the disallowedIncremental marker.
   
   But this method would only check the full backup at time 1. Correct behavior would be to check all FULL backups (from newest to oldest) up until all tables present in the backup root are covered. `TableBackupClient#getAncestors` might be useful here.
   
   Realizing the complexity here, we do have 2 options:
   - Current approach - meaning that creating a single FULL backup to T2 would re-enable incremental backups.
   - Somewhat simpler approach, though with more code changes - we could move the "disallow incrementals" to the backuproot-level data (similar to eg backupStartCode). This would mean only a single GET has to be done in order to find out whether or not incrementals are allowed.



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -277,6 +277,41 @@ public void close() {
     // do nothing
   }
 
+  /**
+   * @param toDisallow Any most recent full back containing this table will be marked as disallowing

Review Comment:
   This line makes little sense. Typos?



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupsDisallowedException.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup.impl;
+
+import org.apache.hadoop.hbase.HBaseIOException;
+import org.apache.hadoop.hbase.backup.BackupRequest;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public class IncrementalBackupsDisallowedException extends HBaseIOException {
+  public IncrementalBackupsDisallowedException(BackupRequest request) {

Review Comment:
   For helping API users deal with this exception, I'd put the table list as an accessible field in the exception.
   
   So a user could do:
   ```
   try {
     makeHbaseIncrBackup();
   } catch (IncrementalBackupsDisallowedException e) {
     makeHbaseFullBackup(e.tables());
     makeHbaseIncrBackup();
   }
   ```



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -840,6 +875,24 @@ public ArrayList<BackupInfo> getBackupInfos(BackupState state) throws IOExceptio
     }
   }
 
+  private List<BackupInfo> getCompletedFullBackupsSortedByHistoryDesc() throws IOException {

Review Comment:
   Better to re-use `BackupSystemTable#getBackupInfos`.
   
   Note that the backupInfos are sorted naturally due to the scanning order.



##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupSystemTable.java:
##########
@@ -479,6 +482,49 @@ public void testBackupSetList() throws IOException {
     }
   }
 
+  @Test
+  public void testDisallowFurtherIncrementals() throws Exception {
+    try (BackupSystemTable table = new BackupSystemTable(conn)) {
+      TableName toInvalidate = TableName.valueOf("t1");
+      List<TableName> t1 = Lists.newArrayList(toInvalidate, TableName.valueOf("t2"));

Review Comment:
   Your naming of the lists is a bit confusing. Suggestion: `t1_t2`, `t1_t3`, ...



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -277,6 +277,41 @@ public void close() {
     // do nothing
   }
 
+  /**
+   * @param toDisallow Any most recent full back containing this table will be marked as disallowing
+   *                   further incrementals
+   */
+  public void disallowFurtherIncrementals(TableName toDisallow) throws IOException {
+    List<BackupInfo> fullTableBackups = getCompletedFullBackupsSortedByHistoryDesc();
+    List<Put> invalidatePuts = new ArrayList<>(fullTableBackups.size());
+    Set<String> backupRootDirsSeen = new HashSet<>(fullTableBackups.size());
+
+    for (BackupInfo backupInfo : fullTableBackups) {
+      // to minimize the amount of mutations against the backup system table, we only
+      // need to update the most recent full backups that allow incremental backups
+      if (
+        backupInfo.getTables().contains(toDisallow) && backupInfo.getType() == BackupType.FULL
+          && !backupInfo.isDisallowFurtherIncrementals()

Review Comment:
   This check can be moved to an inner check. In case we truncate the same table multiple times in a row, current behavior will keep adjusting older and older backupInfos. 



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java:
##########
@@ -496,7 +496,7 @@ public static String getTableBackupDir(String backupRootDir, String backupId,
    * @param historyList history list
    * @return sorted list of BackupCompleteData
    */
-  public static ArrayList<BackupInfo> sortHistoryListDesc(ArrayList<BackupInfo> historyList) {
+  public static ArrayList<BackupInfo> sortHistoryListDesc(List<BackupInfo> historyList) {

Review Comment:
   I think this method can just be removed. The list coming from `BackupSystemTable#getBackupInfos` is already sorted due to the scanning order. (If I'm mistaken here, the javadoc for `getBackupInfos` should be updated.)



##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupMasterObserver.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import java.io.IOException;
+import java.util.Optional;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseInterfaceAudience;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupManager;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.coprocessor.MasterCoprocessor;
+import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment;
+import org.apache.hadoop.hbase.coprocessor.MasterObserver;
+import org.apache.hadoop.hbase.coprocessor.ObserverContext;
+import org.apache.yetus.audience.InterfaceAudience;
+
+@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG)
+public class BackupMasterObserver implements MasterObserver, MasterCoprocessor {
+
+  @Override
+  public void preTruncateTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
+    TableName tableName) throws IOException {
+    disallowIncrementalBackups(ctx.getEnvironment(), tableName);
+  }
+
+  @Override
+  public void postTruncateTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
+    TableName tableName) throws IOException {
+    disallowIncrementalBackups(ctx.getEnvironment(), tableName);
+  }
+
+  @Override
+  public Optional<MasterObserver> getMasterObserver() {
+    return Optional.of(this);
+  }
+
+  private static void disallowIncrementalBackups(MasterCoprocessorEnvironment env,
+    TableName tableName) throws IOException {
+    Configuration conf = env.getConfiguration();
+    if (
+      tableName.equals(BackupSystemTable.getTableName(conf)) || !BackupManager.isBackupEnabled(conf)
+    ) {
+      return;
+    }
+
+    BackupSystemTable table = new BackupSystemTable(env.getConnection());
+    try {
+      table.startBackupExclusiveOperation();

Review Comment:
   Hmm, do we actually need to use the lock here? Thinking out loud...
   
   Yes: this prevents a truncation happening while a backup is running, which might cause incomplete data to be stored. In that case, it makes sense that `deleteBulkLoads` is also included in this lock. So the lock should stay outside of `disallowFurtherIncremetnals`.
   
   No: the lock system seems intended to avoid multiple backup operations from overlapping. Using it here would mean it also impacts a non-backup-related system. Is it really bad if a truncate happens during a backup? How is it different from a bulk-load happening or regular mutations during a backup (which is allowed).
   
   Currently hovering towards that we don't need a lock here.



-- 
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