diff --git a/core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java b/core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java index 9aacc7ca18f..c35aca8c0f2 100644 --- a/core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java @@ -198,30 +198,31 @@ public Map getProperties() { var initTableProps = IteratorConfigUtil.getInitialTableProperties(); // check the properties for conflicts with default iterators var defaultIterSettings = IteratorConfigUtil.getInitialTableIteratorSettings(); - // if a default prop already exists, don't want to consider that a conflict - var noDefaultsPropMap = new HashMap<>(propertyMap); - noDefaultsPropMap.entrySet().removeIf(entry -> initTableProps.get(entry.getKey()) != null - && initTableProps.get(entry.getKey()).equals(entry.getValue())); - defaultIterSettings.forEach((setting, scopes) -> { + for (var defaultIterSetting : defaultIterSettings.entrySet()) { + var setting = defaultIterSetting.getKey(); + var scopes = defaultIterSetting.getValue(); try { - TableOperationsHelper.checkIteratorConflicts(noDefaultsPropMap, setting, scopes); + TableOperationsHelper.checkIteratorConflicts(propertyMap, setting, scopes); } catch (AccumuloException e) { - throw new IllegalStateException(String.format( + throw new IllegalArgumentException(String.format( "conflict with default table iterator: scopes: %s setting: %s", scopes, setting), e); } - }); + } // check the properties for conflicts with default properties (non-iterator) var nonIterDefaults = IteratorConfigUtil.getInitialTableProperties(); nonIterDefaults.keySet().removeAll(IteratorConfigUtil.getInitialTableIterators().keySet()); - nonIterDefaults.forEach((dk, dv) -> { + for (var nonIterDefault : nonIterDefaults.entrySet()) { + var dk = nonIterDefault.getKey(); + var dv = nonIterDefault.getValue(); var valInPropMap = propertyMap.get(dk); - Preconditions.checkState(valInPropMap == null || valInPropMap.equals(dv), String.format( + Preconditions.checkArgument(valInPropMap == null || valInPropMap.equals(dv), String.format( "conflict for property %s : %s (default val) != %s (set val)", dk, dv, valInPropMap)); - }); + } propertyMap.putAll(initTableProps); } + return Collections.unmodifiableMap(propertyMap); } diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java index d05e16afc6c..9df25d3251a 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java @@ -32,6 +32,7 @@ import org.apache.accumulo.core.client.admin.NamespaceOperations; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope; +import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; public abstract class NamespaceOperationsHelper implements NamespaceOperations { @@ -146,45 +147,8 @@ public void checkIteratorConflicts(String namespace, IteratorSetting setting, if (!exists(namespace)) { throw new NamespaceNotFoundException(null, namespace, null); } - for (IteratorScope scope : scopes) { - String scopeStr = - String.format("%s%s", Property.TABLE_ITERATOR_PREFIX, scope.name().toLowerCase()); - String nameStr = String.format("%s.%s", scopeStr, setting.getName()); - String optStr = String.format("%s.opt.", nameStr); - Map optionConflicts = new TreeMap<>(); - for (Entry property : this.getProperties(namespace)) { - if (property.getKey().startsWith(scopeStr)) { - if (property.getKey().equals(nameStr)) { - throw new AccumuloException(new IllegalArgumentException("iterator name conflict for " - + setting.getName() + ": " + property.getKey() + "=" + property.getValue())); - } - if (property.getKey().startsWith(optStr)) { - optionConflicts.put(property.getKey(), property.getValue()); - } - if (property.getKey().contains(".opt.")) { - continue; - } - String[] parts = property.getValue().split(","); - if (parts.length != 2) { - throw new AccumuloException("Bad value for existing iterator setting: " - + property.getKey() + "=" + property.getValue()); - } - try { - if (Integer.parseInt(parts[0]) == setting.getPriority()) { - throw new AccumuloException(new IllegalArgumentException( - "iterator priority conflict: " + property.getKey() + "=" + property.getValue())); - } - } catch (NumberFormatException e) { - throw new AccumuloException("Bad value for existing iterator setting: " - + property.getKey() + "=" + property.getValue()); - } - } - } - if (!optionConflicts.isEmpty()) { - throw new AccumuloException(new IllegalArgumentException( - "iterator options conflict for " + setting.getName() + ": " + optionConflicts)); - } - } + var props = this.getNamespaceProperties(namespace); + IteratorConfigUtil.checkIteratorConflicts(props, setting, scopes, true); } @Override diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java index 451496ed9c5..6c7b870a180 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsImpl.java @@ -57,6 +57,7 @@ import org.apache.accumulo.core.data.constraints.Constraint; import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope; import org.apache.accumulo.core.iterators.SortedKeyValueIterator; +import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; import org.apache.accumulo.core.manager.thrift.FateOperation; import org.apache.accumulo.core.rpc.clients.ThriftClientTypes; import org.apache.accumulo.core.trace.TraceUtil; @@ -186,6 +187,8 @@ public void setProperty(final String namespace, final String property, final Str checkArgument(value != null, "value is null"); try { + IteratorConfigUtil.checkIteratorConflicts(tableOps, this, namespace, property, value); + ThriftClientTypes.MANAGER.executeVoidTableCommand(context, client -> client.setNamespaceProperty(TraceUtil.traceInfo(), context.rpcCreds(), namespace, property, value)); @@ -215,6 +218,11 @@ private Map tryToModifyProperties(final String namespace, // from here on the code is assured to always be dealing with the same map. vProperties.setProperties(Map.copyOf(vProperties.getProperties())); + for (var property : vProperties.getProperties().entrySet()) { + IteratorConfigUtil.checkIteratorConflicts(tableOps, this, namespace, property.getKey(), + property.getValue()); + } + try { // Send to server ThriftClientTypes.MANAGER.executeVoidTableCommand(context, @@ -374,6 +382,8 @@ public void attachIterator(String namespace, IteratorSetting setting, throws AccumuloSecurityException, AccumuloException, NamespaceNotFoundException { // testClassLoad validates the namespace name testClassLoad(namespace, setting.getIteratorClass(), SortedKeyValueIterator.class.getName()); + IteratorConfigUtil.checkIteratorConflictsWithTablesInNamespace(tableOps, namespace, setting, + scopes); super.attachIterator(namespace, setting, scopes); } diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java index 030bb958806..6cf7e5e3d78 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java @@ -35,6 +35,7 @@ import org.apache.accumulo.core.client.admin.TableOperations; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope; +import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; public abstract class TableOperationsHelper implements TableOperations { @@ -139,45 +140,7 @@ public static void checkIteratorConflicts(Map props, IteratorSett EnumSet scopes) throws AccumuloException { checkArgument(setting != null, "setting is null"); checkArgument(scopes != null, "scopes is null"); - for (IteratorScope scope : scopes) { - String scopeStr = - String.format("%s%s", Property.TABLE_ITERATOR_PREFIX, scope.name().toLowerCase()); - String nameStr = String.format("%s.%s", scopeStr, setting.getName()); - String optStr = String.format("%s.opt.", nameStr); - Map optionConflicts = new TreeMap<>(); - for (Entry property : props.entrySet()) { - if (property.getKey().startsWith(scopeStr)) { - if (property.getKey().equals(nameStr)) { - throw new AccumuloException(new IllegalArgumentException("iterator name conflict for " - + setting.getName() + ": " + property.getKey() + "=" + property.getValue())); - } - if (property.getKey().startsWith(optStr)) { - optionConflicts.put(property.getKey(), property.getValue()); - } - if (property.getKey().contains(".opt.")) { - continue; - } - String[] parts = property.getValue().split(","); - if (parts.length != 2) { - throw new AccumuloException("Bad value for existing iterator setting: " - + property.getKey() + "=" + property.getValue()); - } - try { - if (Integer.parseInt(parts[0]) == setting.getPriority()) { - throw new AccumuloException(new IllegalArgumentException( - "iterator priority conflict: " + property.getKey() + "=" + property.getValue())); - } - } catch (NumberFormatException e) { - throw new AccumuloException("Bad value for existing iterator setting: " - + property.getKey() + "=" + property.getValue()); - } - } - } - if (!optionConflicts.isEmpty()) { - throw new AccumuloException(new IllegalArgumentException( - "iterator options conflict for " + setting.getName() + ": " + optionConflicts)); - } - } + IteratorConfigUtil.checkIteratorConflicts(props, setting, scopes, true); } @Override diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java index 17727b2b8f1..ac7fe53f8ea 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java @@ -116,6 +116,7 @@ import org.apache.accumulo.core.dataImpl.thrift.TSummaryRequest; import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope; import org.apache.accumulo.core.iterators.SortedKeyValueIterator; +import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; import org.apache.accumulo.core.manager.state.tables.TableState; import org.apache.accumulo.core.manager.thrift.FateOperation; import org.apache.accumulo.core.manager.thrift.FateService; @@ -1008,10 +1009,13 @@ public void setProperty(final String tableName, final String property, final Str checkArgument(value != null, "value is null"); try { + IteratorConfigUtil.checkIteratorConflicts(Map.copyOf(this.getConfiguration(tableName)), + property, value); + setPropertyNoChecks(tableName, property, value); checkLocalityGroups(tableName, property); - } catch (TableNotFoundException e) { + } catch (TableNotFoundException | IllegalArgumentException e) { throw new AccumuloException(e); } } @@ -1022,6 +1026,13 @@ private Map tryToModifyProperties(String tableName, final TVersionedProperties vProperties = ThriftClientTypes.CLIENT.execute(context, client -> client .getVersionedTableProperties(TraceUtil.traceInfo(), context.rpcCreds(), tableName)); + final Map configBeforeMut; + try { + configBeforeMut = getConfiguration(tableName); + } catch (TableNotFoundException e) { + throw new AccumuloException(e); + } + mapMutator.accept(vProperties.getProperties()); // A reference to the map was passed to the user, maybe they still have the reference and are @@ -1030,6 +1041,11 @@ private Map tryToModifyProperties(String tableName, // from here on the code is assured to always be dealing with the same map. vProperties.setProperties(Map.copyOf(vProperties.getProperties())); + for (var property : vProperties.getProperties().entrySet()) { + IteratorConfigUtil.checkIteratorConflicts(configBeforeMut, property.getKey(), + property.getValue()); + } + try { // Send to server ThriftClientTypes.MANAGER.executeVoid(context, diff --git a/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java b/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java index 59224deb9b9..869e09890bf 100644 --- a/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.EnumSet; @@ -31,9 +32,17 @@ import java.util.Map.Entry; import java.util.Set; import java.util.TreeMap; +import java.util.stream.Collectors; import org.apache.accumulo.core.classloader.ClassLoaderUtil; +import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.client.AccumuloSecurityException; import org.apache.accumulo.core.client.IteratorSetting; +import org.apache.accumulo.core.client.NamespaceNotFoundException; +import org.apache.accumulo.core.client.TableNotFoundException; +import org.apache.accumulo.core.client.admin.TableOperations; +import org.apache.accumulo.core.clientImpl.Namespace; +import org.apache.accumulo.core.clientImpl.NamespaceOperationsHelper; import org.apache.accumulo.core.conf.AccumuloConfiguration; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.data.Key; @@ -57,6 +66,19 @@ public class IteratorConfigUtil { public static final Comparator ITER_INFO_COMPARATOR = Comparator.comparingInt(IterInfo::getPriority); + private static final String ITERATOR_PROP_REGEX = + ("^" + Property.TABLE_ITERATOR_PREFIX.getKey() + "(" + Arrays.stream(IteratorScope.values()) + .map(scope -> scope.name().toLowerCase()).collect(Collectors.joining(".|")) + ".)") + .replace(".", "\\.") + "[^.]+$"; + private static final String ITERATOR_PROP_VAL_REGEX = "^[0-9]+,[^,]+$"; + private static final String ITERATOR_PROP_OPT_REGEX = + ("^" + Property.TABLE_ITERATOR_PREFIX.getKey() + "(" + + Arrays.stream(IteratorScope.values()).map(scope -> scope.name().toLowerCase()) + .collect(Collectors.joining(".|")) + + ".)").replace(".", "\\.") + "[^.]+\\.opt\\.[^.]+$"; + private static final String WARNING_MSG = + ". Iterator was set as requested, but may lead to non-deterministic behavior."; + /** * Fetch the correct configuration key prefix for the given scope. Throws an * IllegalArgumentException if no property exists for the given scope. @@ -273,4 +295,216 @@ private static Class> loadClass(boolean useAcc log.trace("Iterator class {} loaded from classpath", iterInfo.className); return clazz; } + + public static void checkIteratorConflicts(Map props, String property, String value) + throws AccumuloException { + if (props.containsKey(property) && props.get(property).equals(value)) { + // setting a property that already exists (i.e., no change) + return; + } + if (isNonOptionIterProp(property, value)) { + String[] iterPropParts = property.split("\\."); + IteratorScope scope = IteratorScope.valueOf(iterPropParts[2]); + String iterName = iterPropParts[3]; + String[] priorityAndClass; + if ((priorityAndClass = value.split(",")).length == 2) { + // given a single property, the only way for the property to be equivalent to an existing + // iterator is if the existing iterator has no options (opts are set as separate props) + IteratorSetting givenIter = new IteratorSetting(Integer.parseInt(priorityAndClass[0]), + iterName, priorityAndClass[1]); + checkIteratorConflicts(props, givenIter, EnumSet.of(scope), false); + } + } + } + + public static void checkIteratorConflicts(TableOperations tableOps, NamespaceOperationsHelper noh, + String namespace, String property, String value) + throws AccumuloException, AccumuloSecurityException, NamespaceNotFoundException { + var props = noh.getNamespaceProperties(namespace); + if (props.containsKey(property) && props.get(property).equals(value)) { + // setting a property that already exists (i.e., no change) + return; + } + + // checking for conflicts in the namespace + if (isNonOptionIterProp(property, value)) { + String[] iterPropParts = property.split("\\."); + IteratorScope scope = IteratorScope.valueOf(iterPropParts[2]); + String iterName = iterPropParts[3]; + String[] priorityAndClass; + if ((priorityAndClass = value.split(",")).length == 2) { + // given a single property, the only way for the property to be equivalent to an existing + // iterator is if the existing iterator has no options (opts are set as separate props) + IteratorSetting givenIter = new IteratorSetting(Integer.parseInt(priorityAndClass[0]), + iterName, priorityAndClass[1]); + checkIteratorConflicts(props, givenIter, EnumSet.of(scope), false); + } + } + + // checking for conflicts for the tables in the namespace + checkIteratorConflictsWithTablesInNamespace(tableOps, namespace, property, value); + } + + public static void checkIteratorConflictsWithTablesInNamespace(TableOperations tableOps, + String namespace, IteratorSetting is, EnumSet scopes) + throws AccumuloException { + Set tablesInNamespace; + if (namespace.equals(Namespace.DEFAULT.name())) { + tablesInNamespace = tableOps.list().stream().filter(t -> !t.contains(Namespace.SEPARATOR)) + .collect(Collectors.toSet()); + } else { + tablesInNamespace = tableOps.list().stream() + .filter(t -> t.startsWith(namespace + Namespace.SEPARATOR)).collect(Collectors.toSet()); + } + try { + for (var table : tablesInNamespace) { + checkIteratorConflicts(tableOps.getTableProperties(table), is, scopes, false); + } + } catch (TableNotFoundException e) { + throw new AccumuloException(e); + } + } + + public static void checkIteratorConflictsWithTablesInNamespace(TableOperations tableOps, + String namespace, String property, String value) throws AccumuloException { + Set tablesInNamespace; + if (namespace.equals(Namespace.DEFAULT.name())) { + tablesInNamespace = tableOps.list().stream().filter(t -> !t.contains(Namespace.SEPARATOR)) + .collect(Collectors.toSet()); + } else { + tablesInNamespace = tableOps.list().stream() + .filter(t -> t.startsWith(namespace + Namespace.SEPARATOR)).collect(Collectors.toSet()); + } + try { + for (var table : tablesInNamespace) { + checkIteratorConflicts(tableOps.getTableProperties(table), property, value); + } + } catch (TableNotFoundException e) { + throw new AccumuloException(e); + } + } + + public static void checkIteratorConflicts(IteratorSetting iterToCheck, + EnumSet iterScopesToCheck, + Map> existingIters, boolean shouldThrow) + throws AccumuloException { + // The reason for the 'shouldThrow' var is to prevent newly added 2.x checks from breaking + // existing user code. Just log the problem and proceed. Major version > 2 will always throw + for (var scope : iterScopesToCheck) { + var existingItersForScope = existingIters.get(scope); + if (existingItersForScope == null) { + continue; + } + for (var existingIter : existingItersForScope) { + // not a conflict if exactly the same + if (iterToCheck.equals(existingIter)) { + continue; + } + if (iterToCheck.getName().equals(existingIter.getName())) { + String msg = + String.format("iterator name conflict at %s scope. %s conflicts with existing %s", + scope, iterToCheck, existingIter); + if (shouldThrow) { + throw new AccumuloException(new IllegalArgumentException(msg)); + } else { + log.warn(msg + WARNING_MSG); + } + } + if (iterToCheck.getPriority() == existingIter.getPriority()) { + String msg = + String.format("iterator priority conflict at %s scope. %s conflicts with existing %s", + scope, iterToCheck, existingIter); + if (shouldThrow) { + throw new AccumuloException(new IllegalArgumentException(msg)); + } else { + log.warn(msg + WARNING_MSG); + } + } + } + } + } + + public static void checkIteratorConflicts(Map props, IteratorSetting iterToCheck, + EnumSet iterScopesToCheck, boolean shouldThrow) throws AccumuloException { + // parse the props map + Map> existingIters = + new HashMap<>(IteratorScope.values().length); + for (var prop : props.entrySet()) { + if (isNonOptionIterProp(prop.getKey(), prop.getValue())) { + var propKeyParts = prop.getKey().split("\\."); + var scope = IteratorScope.valueOf(propKeyParts[2]); + var name = propKeyParts[3]; + var propValParts = prop.getValue().split(","); + var priority = Integer.parseInt(propValParts[0]); + var clazz = propValParts[1]; + var existingIter = + new IteratorSetting(priority, name, clazz, gatherIterOpts(prop.getKey(), props)); + existingIters.computeIfAbsent(scope, s -> new ArrayList<>()).add(existingIter); + } + } + + // check for conflicts + // any iterator option property not part of an existing iterator is an option conflict + for (var prop : props.entrySet()) { + if (isOptionIterProp(prop.getKey())) { + var iterOptPropParts = prop.getKey().split("\\."); + var scope = IteratorScope.valueOf(iterOptPropParts[2]); + var optKey = iterOptPropParts[iterOptPropParts.length - 1]; + var iterName = iterOptPropParts[3]; + if (!existingIters.containsKey(scope) || existingIters.get(scope).stream() + .noneMatch(is -> is.getName().equals(iterName) && is.getOptions().containsKey(optKey) + && is.getOptions().get(optKey).equals(prop.getValue()))) { + String msg = String.format("iterator options conflict for %s : %s=%s", + iterToCheck.getName(), prop.getKey(), prop.getValue()); + if (shouldThrow) { + throw new AccumuloException(new IllegalArgumentException(msg)); + } else { + log.warn(msg + WARNING_MSG); + } + } + } + } + // check if the given iterator conflicts with any existing iterators + checkIteratorConflicts(iterToCheck, iterScopesToCheck, existingIters, shouldThrow); + } + + /** + * Returns true if the property is an iterator property not including iterator option properties + */ + public static boolean isNonOptionIterProp(String propKey, String propVal) { + return propKey.matches(ITERATOR_PROP_REGEX) && propVal.matches(ITERATOR_PROP_VAL_REGEX); + } + + public static boolean isOptionIterProp(String propKey) { + return propKey.matches(ITERATOR_PROP_OPT_REGEX); + } + + public static boolean isIterProp(String propKey, String propVal) { + return isNonOptionIterProp(propKey, propVal) || isOptionIterProp(propKey); + } + + /** + * Returns a new map of all the iterator props contained in the given map + */ + public static Map gatherIteratorProps(Map props) { + Map iterProps = new HashMap<>(); + props.entrySet().stream().filter(entry -> isIterProp(entry.getKey(), entry.getValue())) + .forEach(entry -> iterProps.put(entry.getKey(), entry.getValue())); + return iterProps; + } + + /** + * returns a map of the options associated with the given iterator property key. Options of the + * iterator are obtained by searching the given map + */ + public static Map gatherIterOpts(String iterPropKey, Map map) { + Map opts = new HashMap<>(); + for (var iteratorProp : map.entrySet()) { + if (isOptionIterProp(iteratorProp.getKey()) && iteratorProp.getKey().contains(iterPropKey)) { + String[] parts = iteratorProp.getKey().split("\\."); + opts.put(parts[parts.length - 1], iteratorProp.getValue()); + } + } + return opts; + } } diff --git a/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java b/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java index d0699e8a1d7..97917015719 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java +++ b/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java @@ -25,6 +25,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; +import java.util.Set; import java.util.stream.Collectors; import org.apache.accumulo.core.classloader.ClassLoaderUtil; @@ -116,6 +117,8 @@ public static class ParsedIteratorConfig { private final List tableIters; private final Map> tableOpts; private final String context; + private final Set uniqueNames; + private final Set uniquePriorities; private ParsedIteratorConfig(List ii, Map> opts, String context) { @@ -123,6 +126,10 @@ private ParsedIteratorConfig(List ii, Map> o tableOpts = opts.entrySet().stream() .collect(Collectors.toUnmodifiableMap(Entry::getKey, e -> Map.copyOf(e.getValue()))); this.context = context; + uniqueNames = + tableIters.stream().map(IterInfo::getIterName).collect(Collectors.toUnmodifiableSet()); + uniquePriorities = + tableIters.stream().map(IterInfo::getPriority).collect(Collectors.toUnmodifiableSet()); } public List getIterInfo() { @@ -136,6 +143,14 @@ public Map> getOpts() { public String getServiceEnv() { return context; } + + public Set getUniqueNames() { + return uniqueNames; + } + + public Set getUniquePriorities() { + return uniquePriorities; + } } public ParsedIteratorConfig getParsedIteratorConfig(IteratorScope scope) { diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java index 57e3858a2da..d0ee1e9a123 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java @@ -44,7 +44,9 @@ import java.util.Set; import java.util.stream.Collectors; +import org.apache.accumulo.core.client.AccumuloException; import org.apache.accumulo.core.client.AccumuloSecurityException; +import org.apache.accumulo.core.client.IteratorSetting; import org.apache.accumulo.core.client.NamespaceNotFoundException; import org.apache.accumulo.core.client.TableNotFoundException; import org.apache.accumulo.core.client.admin.CompactionConfig; @@ -63,6 +65,8 @@ import org.apache.accumulo.core.data.NamespaceId; import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.fate.ReadOnlyTStore.TStatus; +import org.apache.accumulo.core.iterators.IteratorUtil; +import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; import org.apache.accumulo.core.manager.state.tables.TableState; import org.apache.accumulo.core.manager.thrift.FateOperation; import org.apache.accumulo.core.manager.thrift.FateService; @@ -216,16 +220,12 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe throw new ThriftSecurityException(c.getPrincipal(), SecurityErrorCode.PERMISSION_DENIED); } + var namespaceIterProps = IteratorConfigUtil + .gatherIteratorProps(manager.getContext().getNamespaceConfiguration(namespaceId) + .getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX)); for (Map.Entry entry : options.entrySet()) { - if (!Property.isValidProperty(entry.getKey(), entry.getValue())) { - String errorMessage = "Property or value not valid "; - if (!Property.isValidTablePropertyKey(entry.getKey())) { - errorMessage = "Invalid Table Property "; - } - throw new ThriftTableOperationException(null, tableName, tableOp, - TableOperationExceptionType.OTHER, - errorMessage + entry.getKey() + "=" + entry.getValue()); - } + validateTableProperty(entry.getKey(), entry.getValue(), options, namespaceIterProps, + tableName, tableOp); } goalMessage += "Create table " + tableName + " " + initialTableState + " with " + splitCount @@ -322,6 +322,18 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe Map propertiesToSet = new HashMap<>(); Set propertiesToExclude = new HashSet<>(); + // dest table will have the dest namespace props + src table props: need to check provided + // options to set for conflicts with this + var srcTableConfigIterProps = + new HashMap<>(manager.getContext().getTableConfiguration(srcTableId) + .getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX)); + var srcNamespaceConfigIterProps = + manager.getContext().getNamespaceConfiguration(srcNamespaceId) + .getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX); + srcNamespaceConfigIterProps.forEach((k, v) -> srcTableConfigIterProps.remove(k)); + var iterProps = new HashMap<>(manager.getContext().getNamespaceConfiguration(namespaceId) + .getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX)); + iterProps.putAll(srcTableConfigIterProps); for (Entry entry : options.entrySet()) { if (entry.getKey().startsWith(TableOperationsImpl.PROPERTY_EXCLUDE_PREFIX)) { propertiesToExclude.add( @@ -329,15 +341,8 @@ public void executeFateOperation(TInfo tinfo, TCredentials c, long opid, FateOpe continue; } - if (!Property.isValidProperty(entry.getKey(), entry.getValue())) { - String errorMessage = "Property or value not valid "; - if (!Property.isValidTablePropertyKey(entry.getKey())) { - errorMessage = "Invalid Table Property "; - } - throw new ThriftTableOperationException(null, tableName, tableOp, - TableOperationExceptionType.OTHER, - errorMessage + entry.getKey() + "=" + entry.getValue()); - } + validateTableProperty(entry.getKey(), entry.getValue(), options, + IteratorConfigUtil.gatherIteratorProps(iterProps), tableName, tableOp); propertiesToSet.put(entry.getKey(), entry.getValue()); } @@ -844,6 +849,38 @@ private void writeSplitsToFile(Path splitsPath, final List arguments } } + private void validateTableProperty(String propKey, String propVal, Map propMap, + Map config, String tableName, TableOperation tableOp) + throws ThriftTableOperationException { + // validating property as valid table property + if (!Property.isValidProperty(propKey, propVal)) { + String errorMessage = "Property or value not valid "; + if (!Property.isValidTablePropertyKey(propKey)) { + errorMessage = "Invalid Table Property "; + } + throw new ThriftTableOperationException(null, tableName, tableOp, + TableOperationExceptionType.OTHER, errorMessage + propKey + "=" + propVal); + } + + // validating property does not create an iterator conflict with those in the config + if (IteratorConfigUtil.isNonOptionIterProp(propKey, propVal)) { + var iterPropKeyParts = propKey.split("\\."); + var iterPropValParts = propVal.split(","); + String iterName = iterPropKeyParts[iterPropKeyParts.length - 1]; + IteratorUtil.IteratorScope iterScope = + IteratorUtil.IteratorScope.valueOf(iterPropKeyParts[iterPropKeyParts.length - 2]); + Map opts = IteratorConfigUtil.gatherIterOpts(propKey, propMap); + var is = new IteratorSetting(Integer.parseInt(iterPropValParts[0]), iterName, + iterPropValParts[1], opts); + try { + IteratorConfigUtil.checkIteratorConflicts(config, is, EnumSet.of(iterScope), false); + } catch (AccumuloException e) { + throw new ThriftTableOperationException(null, tableName, tableOp, + TableOperationExceptionType.OTHER, e.getMessage()); + } + } + } + /** * Creates a temporary directory for the given FaTE operation (deleting any existing, to avoid * issues in case of server retry). diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java index 13c75157c63..f999feff0c3 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java @@ -20,12 +20,15 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.EnumSet; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.client.IteratorSetting; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.data.Key; import org.apache.accumulo.core.data.Value; @@ -243,6 +246,32 @@ private SortedKeyValueIterator createIterator() } else { // Scan time iterator options were set, so need to merge those with pre-parsed table // iterator options. + + // First ensure the set iterators do not conflict with the existing table iterators. + List picIteratorSettings = null; + for (var scanParamIterInfo : scanParams.getSsiList()) { + // Quick check for a potential iterator conflict (does not consider iterator scope). + // This avoids the more expensive check method call most of the time. + if (pic.getUniqueNames().contains(scanParamIterInfo.getIterName()) + || pic.getUniquePriorities().contains(scanParamIterInfo.getPriority())) { + if (picIteratorSettings == null) { + picIteratorSettings = new ArrayList<>(pic.getIterInfo().size()); + for (var picIterInfo : pic.getIterInfo()) { + picIteratorSettings.add( + getIteratorSetting(picIterInfo, pic.getOpts().get(picIterInfo.getIterName()))); + } + } + try { + IteratorConfigUtil.checkIteratorConflicts( + getIteratorSetting(scanParamIterInfo, + scanParams.getSsio().get(scanParamIterInfo.getIterName())), + EnumSet.of(IteratorScope.scan), Map.of(IteratorScope.scan, picIteratorSettings), + false); + } catch (AccumuloException e) { + throw new IllegalArgumentException(e); + } + } + } iterOpts = new HashMap<>(pic.getOpts().size() + scanParams.getSsio().size()); iterInfos = new ArrayList<>(pic.getIterInfo().size() + scanParams.getSsiList().size()); IteratorConfigUtil.mergeIteratorConfig(iterInfos, iterOpts, pic.getIterInfo(), @@ -272,6 +301,18 @@ private SortedKeyValueIterator createIterator() } } + private IteratorSetting getIteratorSetting(IterInfo iterInfo, Map iterOpts) { + IteratorSetting setting; + if (iterOpts != null) { + setting = new IteratorSetting(iterInfo.getPriority(), iterInfo.getIterName(), + iterInfo.getClassName(), iterOpts); + } else { + setting = new IteratorSetting(iterInfo.getPriority(), iterInfo.getIterName(), + iterInfo.getClassName()); + } + return setting; + } + private void returnIterators() { if (memIters != null) { log.trace("Returning mem iterators for {}, scanId:{}, fid:{}", tablet.getExtent(), diff --git a/test/pom.xml b/test/pom.xml index 4430f6d929e..30f5cf3d3c9 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -182,6 +182,14 @@ org.apache.logging.log4j log4j-1.2-api + + org.apache.logging.log4j + log4j-api + + + org.apache.logging.log4j + log4j-core + org.apache.thrift libthrift diff --git a/test/src/main/java/org/apache/accumulo/test/NewTableConfigurationIT.java b/test/src/main/java/org/apache/accumulo/test/NewTableConfigurationIT.java index bf044eb3857..4c99ab36605 100644 --- a/test/src/main/java/org/apache/accumulo/test/NewTableConfigurationIT.java +++ b/test/src/main/java/org/apache/accumulo/test/NewTableConfigurationIT.java @@ -534,12 +534,12 @@ public void testConflictsWithDefaults() throws Exception { */ // add an iterator with same priority as the default iterator var iterator1 = new IteratorSetting(20, "foo", "foo.bar"); - var exception = assertThrows(IllegalStateException.class, () -> client.tableOperations() + var exception = assertThrows(IllegalArgumentException.class, () -> client.tableOperations() .create(table, new NewTableConfiguration().attachIterator(iterator1))); assertTrue(exception.getMessage().contains("conflict with default table iterator")); // add an iterator with same name as the default iterator var iterator2 = new IteratorSetting(10, "vers", "foo.bar"); - exception = assertThrows(IllegalStateException.class, () -> client.tableOperations() + exception = assertThrows(IllegalArgumentException.class, () -> client.tableOperations() .create(table, new NewTableConfiguration().attachIterator(iterator2))); assertTrue(exception.getMessage().contains("conflict with default table iterator")); // try to attach the exact default iterators, should not present a conflict as they are @@ -561,7 +561,7 @@ public void testConflictsWithDefaults() throws Exception { for (IteratorScope iterScope : IteratorScope.values()) { props.put(Property.TABLE_ITERATOR_PREFIX + iterScope.name() + ".foo", "20,foo.bar"); } - exception = assertThrows(IllegalStateException.class, () -> client.tableOperations() + exception = assertThrows(IllegalArgumentException.class, () -> client.tableOperations() .create(table2, new NewTableConfiguration().setProperties(props))); assertTrue(exception.getMessage().contains("conflict with default table iterator")); props.clear(); @@ -569,7 +569,7 @@ public void testConflictsWithDefaults() throws Exception { for (IteratorScope iterScope : IteratorScope.values()) { props.put(Property.TABLE_ITERATOR_PREFIX + iterScope.name() + ".vers", "10,foo.bar"); } - exception = assertThrows(IllegalStateException.class, () -> client.tableOperations() + exception = assertThrows(IllegalArgumentException.class, () -> client.tableOperations() .create(table2, new NewTableConfiguration().setProperties(props))); assertTrue(exception.getMessage().contains("conflict with default table iterator")); props.clear(); @@ -583,7 +583,7 @@ public void testConflictsWithDefaults() throws Exception { */ // setting a value different from default should throw props.put(Property.TABLE_CONSTRAINT_PREFIX + "1", "foo"); - exception = assertThrows(IllegalStateException.class, () -> client.tableOperations() + exception = assertThrows(IllegalArgumentException.class, () -> client.tableOperations() .create(table3, new NewTableConfiguration().setProperties(props))); assertTrue(exception.getMessage() .contains("conflict for property " + Property.TABLE_CONSTRAINT_PREFIX + "1")); diff --git a/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java b/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java new file mode 100644 index 00000000000..514305e2955 --- /dev/null +++ b/test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java @@ -0,0 +1,816 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.test.functional; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.BufferedReader; +import java.io.InputStreamReader; +import java.time.LocalDateTime; +import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeParseException; +import java.util.ArrayList; +import java.util.EnumSet; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; + +import org.apache.accumulo.core.client.Accumulo; +import org.apache.accumulo.core.client.AccumuloClient; +import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.client.IteratorSetting; +import org.apache.accumulo.core.client.Scanner; +import org.apache.accumulo.core.client.admin.CloneConfiguration; +import org.apache.accumulo.core.client.admin.NamespaceOperations; +import org.apache.accumulo.core.client.admin.NewTableConfiguration; +import org.apache.accumulo.core.client.admin.TableOperations; +import org.apache.accumulo.core.conf.Property; +import org.apache.accumulo.core.iterators.IteratorUtil; +import org.apache.accumulo.core.iteratorsImpl.IteratorConfigUtil; +import org.apache.accumulo.harness.SharedMiniClusterBase; +import org.apache.hadoop.fs.Path; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.layout.PatternLayout; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; + +/** + * Tests that iterator conflicts are detected and cause exceptions. Iterators can be added multiple + * ways. This test ensures that: + *

+ * - {@link Scanner#addScanIterator(IteratorSetting)} + *

+ * - {@link TableOperations#setProperty(String, String, String)} + *

+ * - {@link TableOperations#modifyProperties(String, Consumer)} + *

+ * - {@link NewTableConfiguration#attachIterator(IteratorSetting, EnumSet)} + *

+ * - {@link TableOperations#attachIterator(String, IteratorSetting, EnumSet)} + *

+ * - {@link NamespaceOperations#attachIterator(String, IteratorSetting, EnumSet)} + *

+ * - {@link NamespaceOperations#setProperty(String, String, String)} + *

+ * - {@link NamespaceOperations#modifyProperties(String, Consumer)} + *

+ * - {@link CloneConfiguration.Builder#setPropertiesToSet(Map)} + *

+ * All fail when conflicts arise from: + *

+ * - Iterators attached directly to a table + *

+ * - Iterators attached to a namespace, inherited by a table + *

+ * - Default table iterators, but should not fail if {@link NewTableConfiguration#withoutDefaults()} + * is specified + *

+ * - Adding the exact iterator already present should not fail + */ +public class IteratorConflictsIT extends SharedMiniClusterBase { + private static TableOperations tops; + private static NamespaceOperations nops; + private static AccumuloClient client; + // doesn't matter what iterator is used here + private static final String iterClass = SlowIterator.class.getName(); + private static final IteratorSetting iter1 = new IteratorSetting(99, "iter1name", iterClass); + private static final String iter1Key = Property.TABLE_ITERATOR_PREFIX + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + "." + iter1.getName(); + private static final String iter1Val = "99," + iterClass; + private static final IteratorSetting iter1PrioConflict = + new IteratorSetting(99, "othername", iterClass); + private static final IteratorSetting iter1NameConflict = + new IteratorSetting(101, iter1.getName(), iterClass); + private static final String iter1PrioConflictKey = Property.TABLE_ITERATOR_PREFIX + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + ".othername"; + private static final String iter1PrioConflictVal = "99," + iterClass; + private static final String iter1NameConflictKey = Property.TABLE_ITERATOR_PREFIX + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + "." + iter1.getName(); + private static final String iter1NameConflictVal = "101," + iterClass; + private static final IteratorSetting defaultIterPrioConflict = + new IteratorSetting(20, "bar", iterClass); + private static final IteratorSetting defaultIterNameConflict = + new IteratorSetting(101, "vers", iterClass); + private static final IteratorSetting defaultTableIter = + IteratorConfigUtil.getInitialTableIteratorSettings().keySet().iterator().next(); + private static final String defaultIterPrioConflictKey = Property.TABLE_ITERATOR_PREFIX + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + ".foo"; + private static final String defaultIterPrioConflictVal = + defaultTableIter.getPriority() + "," + iterClass; + private static final String defaultIterNameConflictKey = Property.TABLE_ITERATOR_PREFIX + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + "." + defaultTableIter.getName(); + private static final String defaultIterNameConflictVal = "99," + iterClass; + private static final String defaultIterKey = Property.TABLE_ITERATOR_PREFIX.getKey() + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + "." + defaultTableIter.getName(); + private static final String defaultIterVal = + defaultTableIter.getPriority() + "," + defaultTableIter.getIteratorClass(); + private static final String defaultIterOptKey = Property.TABLE_ITERATOR_PREFIX.getKey() + + IteratorUtil.IteratorScope.scan.name().toLowerCase() + "." + defaultTableIter.getName() + + ".opt." + defaultTableIter.getOptions().entrySet().iterator().next().getKey(); + private static final String defaultIterOptVal = + defaultTableIter.getOptions().entrySet().iterator().next().getValue(); + + private static final LoggerContext loggerContext = (LoggerContext) LogManager.getContext(false); + private static final Configuration loggerConfig = loggerContext.getConfiguration(); + private static final TestAppender appender = new TestAppender(); + private static final String datePattern = getDatePattern(); + private static final DateTimeFormatter dateTimeFormatter = + DateTimeFormatter.ofPattern(datePattern); + + public static class TestAppender extends AbstractAppender { + // CopyOnWriteArrayList for thread safety, even while iterating + private final List events = new CopyOnWriteArrayList<>(); + + public TestAppender() { + super("TestAppender", null, PatternLayout.createDefaultLayout(), false, null); + } + + @Override + public void append(LogEvent event) { + events.add(event.toImmutable()); + } + + public List events() { + return events; + } + } + + @BeforeAll + public static void startup() throws Exception { + SharedMiniClusterBase.startMiniCluster(); + client = Accumulo.newClient().from(getClientProps()).build(); + tops = client.tableOperations(); + nops = client.namespaceOperations(); + appender.start(); + loggerConfig.getRootLogger().addAppender(appender, Level.WARN, null); + loggerContext.updateLoggers(); + } + + @AfterAll + public static void shutdown() throws Exception { + client.close(); + SharedMiniClusterBase.stopMiniCluster(); + loggerConfig.getRootLogger().removeAppender(appender.getName()); + appender.stop(); + loggerContext.updateLoggers(); + } + + @Test + public void testTableIterConflict() throws Throwable { + final String[] tableNames = getUniqueNames(13); + String table1 = tableNames[0]; + String table2 = tableNames[1]; + String table3 = tableNames[2]; + String table4 = tableNames[3]; + String ns5 = tableNames[4]; + String table5 = ns5 + "." + tableNames[5]; + String ns6 = tableNames[6]; + String table6 = ns6 + "." + tableNames[7]; + String ns7 = tableNames[8]; + String table7 = ns7 + "." + tableNames[9]; + String table8 = tableNames[10]; + for (String ns : List.of(ns5, ns6, ns7)) { + nops.create(ns); + } + for (String table : List.of(table1, table2, table3, table4, table5, table6, table7, table8)) { + tops.create(table); + } + + // testing Scanner.addScanIterator + try (var scanner1 = client.createScanner(table1); var scanner2 = client.createScanner(table1)) { + testTableIterConflict(table1, RuntimeException.class, () -> { + scanner1.addScanIterator(iter1PrioConflict); + scanner1.iterator().hasNext(); + }, () -> { + scanner2.addScanIterator(iter1NameConflict); + scanner2.iterator().hasNext(); + }, false); + } + + // testing TableOperations.setProperty + testTableIterConflict(table2, AccumuloException.class, + () -> tops.setProperty(table2, iter1PrioConflictKey, iter1PrioConflictVal), + () -> tops.setProperty(table2, iter1NameConflictKey, iter1NameConflictVal), false); + + // testing TableOperations.modifyProperties + testTableIterConflict(table3, AccumuloException.class, + () -> tops.modifyProperties(table3, + props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), + () -> tops.modifyProperties(table3, + props -> props.put(iter1NameConflictKey, iter1NameConflictVal)), + false); + + // NewTableConfiguration.attachIterator is not applicable for this test + // Attaching the iterator to the table requires the table to exist, but testing + // NewTableConfiguration.attachIterator requires that the table does not exist + + // testing TableOperations.attachIterator + testTableIterConflict(table4, AccumuloException.class, + () -> tops.attachIterator(table4, iter1PrioConflict), + () -> tops.attachIterator(table4, iter1NameConflict), true); + + // testing NamespaceOperations.attachIterator + testTableIterConflict(table5, AccumuloException.class, + () -> nops.attachIterator(ns5, iter1PrioConflict), + () -> nops.attachIterator(ns5, iter1NameConflict), false); + + // testing NamespaceOperations.setProperty + testTableIterConflict(table6, AccumuloException.class, + () -> nops.setProperty(ns6, iter1PrioConflictKey, iter1PrioConflictVal), + () -> nops.setProperty(ns6, iter1NameConflictKey, iter1NameConflictVal), false); + + // testing NamespaceOperations.modifyProperties + testTableIterConflict(table7, AccumuloException.class, + () -> nops.modifyProperties(ns7, + props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), + () -> nops.modifyProperties(ns7, + props -> props.put(iter1NameConflictKey, iter1NameConflictVal)), + false); + + // testing CloneConfiguration.Builder.setPropertiesToSet + String table9 = tableNames[11]; + String table10 = tableNames[12]; + testTableIterConflict(table8, AccumuloException.class, + () -> tops.clone(table8, table9, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()), + () -> tops.clone(table8, table10, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1NameConflictKey, iter1NameConflictVal)).build()), + false); + } + + private void testTableIterConflict(String table, Class exceptionClass, + Executable iterPrioConflictExec, Executable iterNameConflictExec, boolean shouldThrow) + throws Throwable { + tops.attachIterator(table, iter1); + if (shouldThrow) { + var e = assertThrows(exceptionClass, iterPrioConflictExec); + assertTrue(e.toString().contains("iterator priority conflict")); + e = assertThrows(exceptionClass, iterNameConflictExec); + assertTrue(e.toString().contains("iterator name conflict")); + } else { + assertTrue(logsContain(List.of("iterator priority conflict"), iterPrioConflictExec)); + assertTrue(logsContain(List.of("iterator name conflict"), iterNameConflictExec)); + } + } + + @Test + public void testNamespaceIterConflict() throws Throwable { + final String[] names = getUniqueNames(28); + + // testing Scanner.addScanIterator + String ns1 = names[0]; + nops.create(ns1); + String table1 = ns1 + "." + names[1]; + tops.create(table1); + try (var scanner1 = client.createScanner(table1); var scanner2 = client.createScanner(table1)) { + testNamespaceIterConflict(ns1, RuntimeException.class, () -> { + scanner1.addScanIterator(iter1PrioConflict); + scanner1.iterator().hasNext(); + }, () -> { + scanner2.addScanIterator(iter1NameConflict); + scanner2.iterator().hasNext(); + }, false); + } + + // testing TableOperations.setProperty + String ns2 = names[2]; + nops.create(ns2); + String table2 = ns2 + "." + names[3]; + tops.create(table2); + testNamespaceIterConflict(ns2, AccumuloException.class, + () -> tops.setProperty(table2, iter1PrioConflictKey, iter1PrioConflictVal), + () -> tops.setProperty(table2, iter1NameConflictKey, iter1NameConflictVal), false); + + // testing TableOperations.modifyProperties + String ns3 = names[4]; + nops.create(ns3); + String table3 = ns3 + "." + names[5]; + tops.create(table3); + testNamespaceIterConflict(ns3, AccumuloException.class, + () -> tops.modifyProperties(table3, + props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), + () -> tops.modifyProperties(table3, + props -> props.put(iter1NameConflictKey, iter1NameConflictVal)), + false); + + // testing NewTableConfiguration.attachIterator + String ns4 = names[6]; + nops.create(ns4); + String table4 = ns4 + "." + names[7]; + String table5 = ns4 + "." + names[8]; + testNamespaceIterConflict(ns4, AccumuloException.class, + () -> tops.create(table4, new NewTableConfiguration().attachIterator(iter1PrioConflict)), + () -> tops.create(table5, new NewTableConfiguration().attachIterator(iter1NameConflict)), + false); + + // testing TableOperations.attachIterator + String ns5 = names[9]; + nops.create(ns5); + String table6 = ns5 + "." + names[10]; + tops.create(table6); + testNamespaceIterConflict(ns5, AccumuloException.class, + () -> tops.attachIterator(table6, iter1PrioConflict), + () -> tops.attachIterator(table6, iter1NameConflict), true); + + // testing NamespaceOperations.attachIterator + String ns6 = names[11]; + nops.create(ns6); + testNamespaceIterConflict(ns6, AccumuloException.class, + () -> nops.attachIterator(ns6, iter1PrioConflict), + () -> nops.attachIterator(ns6, iter1NameConflict), true); + + // testing NamespaceOperations.setProperty + String ns7 = names[12]; + nops.create(ns7); + testNamespaceIterConflict(ns7, AccumuloException.class, + () -> nops.setProperty(ns7, iter1PrioConflictKey, iter1PrioConflictVal), + () -> nops.setProperty(ns7, iter1NameConflictKey, iter1NameConflictVal), false); + + // testing NamespaceOperations.modifyProperties + String ns8 = names[13]; + nops.create(ns8); + testNamespaceIterConflict(ns8, AccumuloException.class, + () -> nops.modifyProperties(ns8, + props -> props.put(iter1PrioConflictKey, iter1PrioConflictVal)), + () -> nops.modifyProperties(ns8, + props -> props.put(iter1NameConflictKey, iter1NameConflictVal)), + false); + + // testing CloneConfiguration.Builder.setPropertiesToSet + // testing same src and dst namespace: should conflict + String dstAndSrcNamespace1 = names[14]; + nops.create(dstAndSrcNamespace1); + String src1 = dstAndSrcNamespace1 + "." + names[15]; + tops.create(src1); + String dst1 = dstAndSrcNamespace1 + "." + names[16]; + String dst2 = dstAndSrcNamespace1 + "." + names[17]; + testNamespaceIterConflict(dstAndSrcNamespace1, AccumuloException.class, + () -> tops.clone(src1, dst1, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()), + () -> tops.clone(src1, dst2, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1NameConflictKey, iter1NameConflictVal)).build()), + false); + // testing attached to src namespace, different dst namespace: should not conflict + String srcNamespace2 = names[18]; + nops.create(srcNamespace2); + nops.attachIterator(srcNamespace2, iter1); + String src2 = srcNamespace2 + "." + names[19]; + tops.create(src2); + String dstNamespace2 = names[20]; + nops.create(dstNamespace2); + String dst3 = dstNamespace2 + "." + names[21]; + String dst4 = dstNamespace2 + "." + names[22]; + // should NOT throw + tops.clone(src2, dst3, CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()); + // should NOT throw + tops.clone(src2, dst4, CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1NameConflictKey, iter1NameConflictVal)).build()); + // testing attached to dst namespace, different src namespace: should conflict + String srcNamespace3 = names[23]; + nops.create(srcNamespace3); + String src3 = srcNamespace3 + "." + names[24]; + tops.create(src3); + String dstNamespace3 = names[25]; + nops.create(dstNamespace3); + String dst5 = dstNamespace3 + "." + names[26]; + String dst6 = dstNamespace3 + "." + names[27]; + testNamespaceIterConflict(dstNamespace3, AccumuloException.class, + () -> tops.clone(src3, dst5, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1PrioConflictKey, iter1PrioConflictVal)).build()), + () -> tops.clone(src3, dst6, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(iter1NameConflictKey, iter1NameConflictVal)).build()), + false); + } + + private void testNamespaceIterConflict(String ns, Class exceptionClass, + Executable iterPrioConflictExec, Executable iterNameConflictExec, boolean shouldThrow) + throws Throwable { + nops.attachIterator(ns, iter1); + + if (shouldThrow) { + var e = assertThrows(exceptionClass, iterPrioConflictExec); + assertTrue(e.toString().contains("iterator priority conflict")); + e = assertThrows(exceptionClass, iterNameConflictExec); + assertTrue(e.toString().contains("iterator name conflict")); + } else { + assertTrue(logsContain(List.of("iterator priority conflict"), iterPrioConflictExec)); + assertTrue(logsContain(List.of("iterator name conflict"), iterNameConflictExec)); + } + } + + @Test + public void testDefaultIterConflict() throws Throwable { + final String[] names = getUniqueNames(29); + + // testing Scanner.addScanIterator + String defaultsTable1 = names[0]; + tops.create(defaultsTable1); + String noDefaultsTable1 = names[1]; + tops.create(noDefaultsTable1, new NewTableConfiguration().withoutDefaults()); + try (var defaultsScanner1 = client.createScanner(defaultsTable1); + var noDefaultsScanner1 = client.createScanner(noDefaultsTable1); + var defaultsScanner2 = client.createScanner(defaultsTable1); + var noDefaultsScanner2 = client.createScanner(noDefaultsTable1)) { + testDefaultIterConflict(RuntimeException.class, () -> { + defaultsScanner1.addScanIterator(defaultIterPrioConflict); + defaultsScanner1.iterator().hasNext(); + }, () -> { + defaultsScanner2.addScanIterator(defaultIterNameConflict); + defaultsScanner2.iterator().hasNext(); + }, () -> { + noDefaultsScanner1.addScanIterator(defaultIterPrioConflict); + noDefaultsScanner1.iterator().hasNext(); + }, () -> { + noDefaultsScanner2.addScanIterator(defaultIterNameConflict); + noDefaultsScanner2.iterator().hasNext(); + }, false); + } + + // testing TableOperations.setProperty + String defaultsTable2 = names[2]; + tops.create(defaultsTable2); + String noDefaultsTable2 = names[3]; + tops.create(noDefaultsTable2, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> tops.setProperty(defaultsTable2, defaultIterPrioConflictKey, + defaultIterPrioConflictVal), + () -> tops.setProperty(defaultsTable2, defaultIterNameConflictKey, + defaultIterNameConflictVal), + () -> tops.setProperty(noDefaultsTable2, defaultIterPrioConflictKey, + defaultIterPrioConflictVal), + () -> tops.setProperty(noDefaultsTable2, defaultIterNameConflictKey, + defaultIterNameConflictVal), + false); + + // testing TableOperations.modifyProperties + String defaultsTable3 = names[4]; + tops.create(defaultsTable3); + String noDefaultsTable3 = names[5]; + tops.create(noDefaultsTable3, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> tops.modifyProperties(defaultsTable3, + props -> props.put(defaultIterPrioConflictKey, defaultIterPrioConflictVal)), + () -> tops.modifyProperties(defaultsTable3, + props -> props.put(defaultIterNameConflictKey, defaultIterNameConflictVal)), + () -> tops.modifyProperties(noDefaultsTable3, + props -> props.put(defaultIterPrioConflictKey, defaultIterPrioConflictVal)), + () -> tops.modifyProperties(noDefaultsTable3, + props -> props.put(defaultIterNameConflictKey, defaultIterNameConflictVal)), + false); + + // testing NewTableConfiguration.attachIterator + String defaultsTable4 = names[6]; + String noDefaultsTable4 = names[7]; + String noDefaultsTable5 = names[8]; + testDefaultIterConflict(IllegalArgumentException.class, + () -> tops.create(defaultsTable4, + new NewTableConfiguration().attachIterator(defaultIterPrioConflict)), + () -> tops.create(defaultsTable4, + new NewTableConfiguration().attachIterator(defaultIterNameConflict)), + () -> tops.create(noDefaultsTable4, + new NewTableConfiguration().attachIterator(defaultIterPrioConflict).withoutDefaults()), + () -> tops.create(noDefaultsTable5, + new NewTableConfiguration().attachIterator(defaultIterNameConflict).withoutDefaults()), + true); + + // testing TableOperations.attachIterator + String defaultsTable6 = names[9]; + tops.create(defaultsTable6); + String noDefaultsTable6 = names[10]; + tops.create(noDefaultsTable6, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> tops.attachIterator(defaultsTable6, defaultIterPrioConflict), + () -> tops.attachIterator(defaultsTable6, defaultIterNameConflict), + () -> tops.attachIterator(noDefaultsTable6, defaultIterPrioConflict), + () -> tops.attachIterator(noDefaultsTable6, defaultIterNameConflict), true); + + // testing NamespaceOperations.attachIterator + String ns7 = names[11]; + nops.create(ns7); + String defaultsTable7 = ns7 + "." + names[12]; + tops.create(defaultsTable7); + String ns8 = names[13]; + nops.create(ns8); + String noDefaultsTable8 = ns8 + "." + names[14]; + tops.create(noDefaultsTable8, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> nops.attachIterator(ns7, defaultIterPrioConflict), + () -> nops.attachIterator(ns7, defaultIterNameConflict), + () -> nops.attachIterator(ns8, defaultIterPrioConflict), + () -> nops.attachIterator(ns8, defaultIterNameConflict), false); + + // testing NamespaceOperations.setProperty + String ns9 = names[15]; + nops.create(ns9); + String defaultsTable9 = ns9 + "." + names[16]; + tops.create(defaultsTable9); + String ns10 = names[17]; + nops.create(ns10); + String noDefaultsTable10 = ns10 + "." + names[18]; + tops.create(noDefaultsTable10, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> nops.setProperty(ns9, defaultIterPrioConflictKey, defaultIterPrioConflictVal), + () -> nops.setProperty(ns9, defaultIterNameConflictKey, defaultIterNameConflictVal), + () -> nops.setProperty(ns10, defaultIterPrioConflictKey, defaultIterPrioConflictVal), + () -> nops.setProperty(ns10, defaultIterNameConflictKey, defaultIterNameConflictVal), + false); + + // testing NamespaceOperations.modifyProperties + String ns11 = names[19]; + nops.create(ns11); + String defaultsTable11 = ns11 + "." + names[20]; + tops.create(defaultsTable11); + String ns12 = names[21]; + nops.create(ns12); + String noDefaultsTable12 = ns12 + "." + names[22]; + tops.create(noDefaultsTable12, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> nops.modifyProperties(ns11, + props -> props.put(defaultIterPrioConflictKey, defaultIterPrioConflictVal)), + () -> nops.modifyProperties(ns11, + props -> props.put(defaultIterNameConflictKey, defaultIterNameConflictVal)), + () -> nops.modifyProperties(ns12, + props -> props.put(defaultIterPrioConflictKey, defaultIterPrioConflictVal)), + () -> nops.modifyProperties(ns12, + props -> props.put(defaultIterNameConflictKey, defaultIterNameConflictVal)), + false); + + // testing CloneConfiguration.Builder.setPropertiesToSet + String dst1 = names[23]; + String dst2 = names[24]; + String dst3 = names[25]; + String dst4 = names[26]; + String defaultsTable12 = names[27]; + tops.create(defaultsTable12); + String noDefaultsTable13 = names[28]; + tops.create(noDefaultsTable13, new NewTableConfiguration().withoutDefaults()); + testDefaultIterConflict(AccumuloException.class, + () -> tops + .clone(defaultsTable12, dst1, + CloneConfiguration.builder() + .setPropertiesToSet( + Map.of(defaultIterPrioConflictKey, defaultIterPrioConflictVal)) + .build()), + () -> tops + .clone(defaultsTable12, dst2, + CloneConfiguration.builder() + .setPropertiesToSet( + Map.of(defaultIterNameConflictKey, defaultIterNameConflictVal)) + .build()), + () -> tops + .clone(noDefaultsTable13, dst3, + CloneConfiguration.builder() + .setPropertiesToSet( + Map.of(defaultIterPrioConflictKey, defaultIterPrioConflictVal)) + .build()), + () -> tops.clone(noDefaultsTable13, dst4, + CloneConfiguration.builder() + .setPropertiesToSet(Map.of(defaultIterNameConflictKey, defaultIterNameConflictVal)) + .build()), + false); + } + + private void testDefaultIterConflict(Class exceptionClass, + Executable defaultsTableOp1, Executable defaultsTableOp2, Executable noDefaultsTableOp1, + Executable noDefaultsTableOp2, boolean shouldThrow) throws Throwable { + if (shouldThrow) { + var e = assertThrows(exceptionClass, defaultsTableOp1); + assertTrue(e.toString().contains("conflict with default table iterator") + || e.toString().contains("iterator priority conflict")); + e = assertThrows(exceptionClass, defaultsTableOp2); + assertTrue(e.toString().contains("conflict with default table iterator") + || e.toString().contains("iterator name conflict")); + } else { + assertTrue( + logsContain(List.of("conflict with default table iterator", "iterator priority conflict"), + defaultsTableOp1)); + assertTrue( + logsContain(List.of("conflict with default table iterator", "iterator name conflict"), + defaultsTableOp2)); + } + + noDefaultsTableOp1.execute(); // should NOT fail + noDefaultsTableOp2.execute(); // should NOT fail + } + + @Test + public void testSameIterNoConflict() throws Throwable { + // note about setProperty calls in this test. The default table iter has an option so the + // property representation of this iter is a property for the iter and a property for the + // option (2 properties). Obviously we cannot call setProperty with both of these properties, + // but calling setProperty for one of these properties should be fine as it has no effect on + // the config. + final String[] names = getUniqueNames(16); + + // testing Scanner.addScanIterator + final String table1 = names[0]; + tops.create(table1); + tops.attachIterator(table1, iter1); + try (var scanner1 = client.createScanner(table1); var scanner2 = client.createScanner(table1)) { + testSameIterNoConflict(() -> { + scanner1.addScanIterator(iter1); + scanner1.iterator().hasNext(); + }, () -> { + scanner2.addScanIterator(defaultTableIter); + scanner2.iterator().hasNext(); + }); + } + + // testing TableOperations.setProperty + final String table2 = names[1]; + tops.create(table2); + tops.attachIterator(table2, iter1); + testSameIterNoConflict(() -> tops.setProperty(table2, iter1Key, iter1Val), + () -> tops.setProperty(table2, defaultIterKey, defaultIterVal)); + + // testing TableOperations.modifyProperties + final String table3 = names[2]; + tops.create(table3); + tops.attachIterator(table3, iter1); + testSameIterNoConflict( + () -> tops.modifyProperties(table3, props -> props.put(iter1Key, iter1Val)), + () -> tops.modifyProperties(table3, props -> { + props.put(defaultIterKey, defaultIterVal); + props.put(defaultIterOptKey, defaultIterOptVal); + })); + + // testing NewTableConfiguration.attachIterator + final String ns1 = names[3]; + final String table4 = ns1 + "." + names[4]; + final String table5 = names[5]; + nops.create(ns1); + nops.attachIterator(ns1, iter1); + testSameIterNoConflict( + () -> tops.create(table4, new NewTableConfiguration().attachIterator(iter1)), + () -> tops.create(table5, new NewTableConfiguration().attachIterator(defaultTableIter))); + + // testing TableOperations.attachIterator + final String table6 = names[6]; + tops.create(table6); + tops.attachIterator(table6, iter1); + testSameIterNoConflict(() -> tops.attachIterator(table6, iter1), + () -> tops.attachIterator(table6, defaultTableIter)); + + // testing NamespaceOperations.attachIterator + final String ns2 = names[7]; + final String table7 = ns2 + "." + names[8]; + nops.create(ns2); + tops.create(table7); + tops.attachIterator(table7, iter1); + testSameIterNoConflict(() -> nops.attachIterator(ns2, iter1), + () -> nops.attachIterator(ns2, defaultTableIter)); + + // testing NamespaceOperations.setProperty + final String ns3 = names[9]; + final String table8 = ns3 + "." + names[10]; + nops.create(ns3); + tops.create(table8); + tops.attachIterator(table8, iter1); + testSameIterNoConflict(() -> nops.setProperty(ns3, iter1Key, iter1Val), + () -> nops.setProperty(ns3, defaultIterKey, defaultIterVal)); + + // testing NamespaceOperations.modifyProperties + final String ns4 = names[11]; + final String table9 = ns4 + "." + names[12]; + nops.create(ns4); + tops.create(table9); + tops.attachIterator(table9, iter1); + testSameIterNoConflict(() -> nops.modifyProperties(ns4, props -> props.put(iter1Key, iter1Val)), + () -> nops.modifyProperties(ns4, props -> { + props.put(defaultIterKey, defaultIterVal); + props.put(defaultIterOptKey, defaultIterOptVal); + })); + + // testing CloneConfiguration.Builder.setPropertiesToSet + final String src = names[13]; + final String dst1 = names[14]; + final String dst2 = names[15]; + tops.create(src); + tops.attachIterator(src, iter1); + testSameIterNoConflict( + () -> tops.clone(src, dst1, + CloneConfiguration.builder().setPropertiesToSet(Map.of(iter1Key, iter1Val)).build()), + () -> tops.clone(src, dst2, + CloneConfiguration.builder() + .setPropertiesToSet( + Map.of(defaultIterKey, defaultIterVal, defaultIterOptKey, defaultIterOptVal)) + .build())); + } + + private void testSameIterNoConflict(Executable addIter1Executable, + Executable addDefaultIterExecutable) throws Throwable { + // should be able to add same exact iterator + addIter1Executable.execute(); + addDefaultIterExecutable.execute(); + } + + private static boolean logsContain(List expectedStrs, Executable exec) throws Throwable { + var timeBeforeExec = LocalDateTime.now(); + var timeBeforeExecMillis = System.currentTimeMillis(); + exec.execute(); + + // check the logs from other processes for a log that occurred after the execution and + // contains one of the expected strings + List warnLogsAfterExec = warnLogsAfter(timeBeforeExec); + for (var warnLog : warnLogsAfterExec) { + if (expectedStrs.stream().anyMatch(warnLog::contains)) { + return true; + } + } + + // check the logs from the test process (this process) for a log that occurred after the + // execution and contains one of the expected strings + return appender.events().stream() + .anyMatch(logEvent -> logEvent.getTimeMillis() > timeBeforeExecMillis && expectedStrs + .stream().anyMatch(logEvent.getMessage().getFormattedMessage()::contains)); + } + + private static String getDatePattern() { + String datePattern = null; + for (var appender : loggerConfig.getAppenders().values()) { + if (appender.getLayout() instanceof PatternLayout) { + PatternLayout layout = (PatternLayout) appender.getLayout(); + String pattern = layout.getConversionPattern(); + if (pattern.contains("%d{ISO8601}")) { + datePattern = "yyyy-MM-dd'T'HH:mm:ss,SSS"; + break; + } + } + } + assertNotNull(datePattern, + "Format of dates in log4j config has changed. This test needs to be updated"); + return datePattern; + } + + private static List warnLogsAfter(LocalDateTime timeBeforeExec) throws Exception { + var filesIter = getCluster().getFileSystem() + .listFiles(new Path(getCluster().getConfig().getLogDir().toURI()), false); + List files = new ArrayList<>(); + List lines = new ArrayList<>(); + + // get all the WARN logs in the Manager and TabletServer logs that happened after the given + // time. We only care about the Manager and TabletServer as these are the only servers that + // will check for iterator conflicts + while (filesIter.hasNext()) { + var file = filesIter.next(); + if (file.getPath().getName().matches("(Manager_|TabletServer_).+\\.out")) { + files.add(file.getPath()); + } + } + for (var path : files) { + try (var in = getCluster().getFileSystem().open(path); + BufferedReader reader = new BufferedReader(new InputStreamReader(in, UTF_8))) { + String line; + while ((line = reader.readLine()) != null) { + if (line.contains("WARN")) { + var words = line.split(" "); + try { + if (words.length >= 1 + && LocalDateTime.parse(words[0], dateTimeFormatter).isAfter(timeBeforeExec)) { + lines.add(line); + } + } catch (DateTimeParseException e) { + // ignore + } + } + } + } + } + + return lines; + } +}