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