[GitHub] [hbase] joshelser commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot

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

joshelser commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r765345147



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
    */
   default void cloneSnapshot(String snapshotName, TableName tableName)
       throws IOException, TableExistsException, RestoreSnapshotException {
-    cloneSnapshot(snapshotName, tableName, false);
+    cloneSnapshot(snapshotName, tableName, false, null);
+  }
+
+  /**
+   * Create a new table by cloning the snapshot content.
+   * @param snapshotName name of the snapshot to be cloned
+   * @param tableName name of the table where the snapshot will be restored
+   * @param restoreAcl <code>true</code> to clone acl into newly created table
+   * @param customSFT specify the StroreFileTracker used for the table

Review comment:
       ```suggestion
      * @param customSFT specify the StoreFileTracker used for the table
   ```

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
    */
   default void cloneSnapshot(String snapshotName, TableName tableName)
       throws IOException, TableExistsException, RestoreSnapshotException {
-    cloneSnapshot(snapshotName, tableName, false);
+    cloneSnapshot(snapshotName, tableName, false, null);
+  }
+
+  /**
+   * Create a new table by cloning the snapshot content.
+   * @param snapshotName name of the snapshot to be cloned
+   * @param tableName name of the table where the snapshot will be restored
+   * @param restoreAcl <code>true</code> to clone acl into newly created table
+   * @param customSFT specify the StroreFileTracker used for the table
+   * @throws IOException if a remote or network exception occurs
+   * @throws TableExistsException if table to be created already exists
+   * @throws RestoreSnapshotException if snapshot failed to be cloned
+   * @throws IllegalArgumentException if the specified table has not a valid name
+   */
+  default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+    String customSFT)

Review comment:
       Admin is public API, so this is a commitment that using a String for SFT is the path forward. Do we want a String vs. an enum? I'm not positive if our rules say that we can mark just this one method as private/unstable until we are sure SFT is ready for all users.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,26 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
     return Flow.HAS_MORE_STATE;
   }
 
+  /**
+   * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+   * and set the specified SFT on the table level
+   */
+  private void updateTableDescriptorWithSFT() {
+    if (StringUtils.isEmpty(customSFT)){
+      return;
+    }
+
+    TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+    builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);

Review comment:
       Do we have any ability to validate that this is a valid `customSFT` prior to modifying the TableDesc? Maybe a call we could make on `StoreFileTrackerFactory`?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
    */
   default void cloneSnapshot(String snapshotName, TableName tableName)
       throws IOException, TableExistsException, RestoreSnapshotException {
-    cloneSnapshot(snapshotName, tableName, false);
+    cloneSnapshot(snapshotName, tableName, false, null);
+  }
+
+  /**
+   * Create a new table by cloning the snapshot content.
+   * @param snapshotName name of the snapshot to be cloned
+   * @param tableName name of the table where the snapshot will be restored
+   * @param restoreAcl <code>true</code> to clone acl into newly created table
+   * @param customSFT specify the StroreFileTracker used for the table
+   * @throws IOException if a remote or network exception occurs
+   * @throws TableExistsException if table to be created already exists
+   * @throws RestoreSnapshotException if snapshot failed to be cloned
+   * @throws IllegalArgumentException if the specified table has not a valid name
+   */
+  default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+    String customSFT)

Review comment:
       Ok, Admin is IA.Public which requires it to be IS.Stable. Rereading the hbase book, I don't think there's anything stopping us from claiming this one method is IA.LimitedPrivate/IS.Evolving. Maybe someone else can verify my thinking.

##########
File path: hbase-shell/src/main/ruby/shell/commands/clone_snapshot.rb
##########
@@ -33,13 +33,17 @@ def help
 newly created table.
 
   hbase> clone_snapshot 'snapshotName', 'namespace:tableName', {RESTORE_ACL=>true}
+
+StoreFileTracker implementation used after restore can be set by the following command.
+  hbase> clone_snapshot 'snapshotName', 'namespace:tableName', {CLONE_SFT=>FILE}

Review comment:
       should be `{CLONE_SFT=>"FILE"}`?




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