[GitHub] [hbase] Apache9 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>]

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



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,24 @@ 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 cloneSFT specify the StroreFileTracker implementation used for the table

Review comment:
       The parameter name is a bit confusing to me. It seems to mean whether to clone the SFT as clone is a verb...
   Maybe just call it storeFileTrackerImpl?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java
##########
@@ -488,14 +488,15 @@
 
   @Override
   public CompletableFuture<Void> restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot,
-      boolean restoreAcl) {
-    return wrap(rawAdmin.restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl));
+    boolean restoreAcl) {
+    return wrap(

Review comment:
       Do we need this change?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##########
@@ -644,14 +644,14 @@ public void restoreSnapshot(String snapshotName) throws IOException, RestoreSnap
 
   @Override
   public void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean restoreAcl)
-      throws IOException, RestoreSnapshotException {
+    throws IOException, RestoreSnapshotException {
     get(admin.restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl));
   }
 
-  @Override
-  public Future<Void> cloneSnapshotAsync(String snapshotName, TableName tableName,
-      boolean restoreAcl) throws IOException, TableExistsException, RestoreSnapshotException {
-    return admin.cloneSnapshot(snapshotName, tableName, restoreAcl);
+  @Override public Future<Void> cloneSnapshotAsync(String snapshotName, TableName tableName,

Review comment:
       Nits: `@Override` in a separated line

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -71,6 +75,7 @@
   private TableDescriptor tableDescriptor;
   private SnapshotDescription snapshot;
   private boolean restoreAcl;
+  private String cloneSFT;

Review comment:
       I think we need to persist this in the procedure state data?




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