BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753396530
##########
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:
Generally I would agree and I started to write it like that, breaking it
down to single checks, but it got huge and from all the numerous outcomes only
3 was interesting to us. Consider the first part:
`if (StringUtils.isEmpty(currentValue)) {
if (!StringUtils.isEmpty(newValue)) {
if (!defaultValue.equals(newValue)){
hasIncompatibility = true;
}
else {
//do nothing
}
}
else {
//do nothing
}
}
`
I never use the else cases and only care if all 3 conditions are true. At
that point it felt cleaner to just make it a single check with all 3 conditions.
Alternately I could do something like:
`if (StringUtils.isEmpty(currentValue)) {
if (!StringUtils.isEmpty(newValue) && !defaultValue.equals(newValue)) {
hasIncompatibility = true;
}
}
//if there is a current override
else if (!StringUtils.isEmpty(currentValue)) {
// we can only remove the override if it matches the default
if (StringUtils.isEmpty(newValue)) {
if (!defaultValue.equals(currentValue)){
hasIncompatibility = true;
}
}
// the new value have to match the current one
else if (!StringUtils.isEmpty(newValue)) {
if (!currentValue.equals(newValue)) {
hasIncompatibility = true;
}
}
}`
But even here we have if statements that have no else cause so they could be
just merged back to check above them like I originally did.
What do you think?
--
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-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
