Prevents iterator conflicts

core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java

@@ -198,30 +198,31 @@ public Map<String,String> 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);
   }
 

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<String,String> optionConflicts = new TreeMap<>();
-      for (Entry<String,String> 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

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<String,String> 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);
   }
 

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<String,String> props, IteratorSett
       EnumSet<IteratorScope> 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<String,String> optionConflicts = new TreeMap<>();
-      for (Entry<String,String> 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

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<String,String> tryToModifyProperties(String tableName,
     final TVersionedProperties vProperties =
         ThriftClientTypes.CLIENT.execute(context, client -> client
             .getVersionedTableProperties(TraceUtil.traceInfo(), context.rpcCreds(), tableName));
+    final Map<String,String> 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<String,String> 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,