Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r767920480
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/RestoreSnapshotHelper.java
##########
@@ -707,7 +706,9 @@ private void cloneRegion(final RegionInfo newRegionInfo,
final Path regionDir,
HRegionFileSystem.openRegionFromFileSystem(conf, fs, tableDir,
newRegionInfo, false) :
HRegionFileSystem.createRegionOnFileSystem(conf, fs, tableDir,
newRegionInfo);
- StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true,
+ Configuration sftConf = StoreUtils.createStoreConfiguration(conf,
tableDesc,
Review comment:
Nice catch.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -92,7 +92,7 @@ static String getStoreFileTrackerName(Class<? extends
StoreFileTracker> clazz) {
return name != null ? name.name() : clazz.getName();
}
- private static Class<? extends StoreFileTracker>
getTrackerClass(Configuration conf) {
+ public static Class<? extends StoreFileTracker>
getTrackerClass(Configuration conf) {
Review comment:
So if we move the method to this class, then we do not need to change
this modifier.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +859,46 @@ public long restoreOrCloneSnapshot(final
SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot,
snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc,
nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc,
nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc,
nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
Review comment:
I think we have several similar methods in StoreFileTrackerFactory class
https://github.com/apache/hbase/blob/4aa3f47aa295d0c4bd6235c6bc63897136fa9278/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L196
https://github.com/apache/hbase/blob/4aa3f47aa295d0c4bd6235c6bc63897136fa9278/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L238
So let's also move this method there? Just call it checkForRestoreSnapshot?
##########
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:
For IA.Public interface, all methods in it should be considered
IS.Stable. The IS annotation is only useful when the class is
IA.LimitedPrivate. For IA.Private, everything is IS.Unstable. That's my
understanding.
And I think a String is OK here, we could do some checks at server side when
we actually convert it to a SFT instance, and report back to client if there is
something wrong.
##########
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:
I think we should do this check prior to do any real operations? In
prepareClone method? In ModifyTableProcedure, we do the check in prepareModify
method.
--
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]
