Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753632988
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ 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,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws
RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault =
checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL,
StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies())
{
+ ColumnFamilyDescriptor snapCFDesc =
snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so
the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+
snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+
checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // check if a config change would change the behavior
+ static String checkIncompatibleConfig(String currentValue, String newValue,
String defaultValue,
+ String errorMessage) throws RestoreSnapshotException {
+ Boolean hasIncompatibility = false;
+ //if there is no current override and the snapshot has an override that
does not match the
+ //default
+ if (StringUtils.isEmpty(currentValue) && !StringUtils.isEmpty(newValue) &&
Review comment:
OK, reviewed the code again, I think the difficulty here is that, we are
not comparing only two values, there is a default value too. Then I suggest you
use the trick in the checking method in StoreFileTrackerFactory, such as
https://github.com/apache/hbase/blob/0ad3556da7e71b4719ee41de8fb704b45b390d00/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L238
Where we merge the base configuration with table descriptor and cf
descriptor first, and then get the store file tracker implementation from the
merged configuration object. In this way we do not need to take care of the
override logic as the mergeConfigurations method does it for us.
Of course it will be slower as we need to merge all the configuration
values, not only for store file tracker, but I do not think speed of this check
is the bottleneck of a restoreSnapshot operation.
--
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]
