kevinrr888 commented on code in PR #6040: URL: https://github.com/apache/accumulo/pull/6040#discussion_r2669856999
########## test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java: ########## @@ -0,0 +1,513 @@ +/* + * 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 org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.EnumSet; +import java.util.List; +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.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.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: + * <p> + * - {@link Scanner#addScanIterator(IteratorSetting)} + * <p> Review Comment: The iterator conflict tests I added for addScanIterator really only test ScannerImpl impl of ScannerOptions not sure if its worthwhile to add testing for all impls ########## core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java: ########## @@ -199,29 +215,51 @@ public Map<String,String> getProperties() { // 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 AccumuloException(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( - "conflict for property %s : %s (default val) != %s (set val)", dk, dv, valInPropMap)); - }); + if (valInPropMap != null && !valInPropMap.equals(dv)) { + throw new AccumuloException(String.format( + "conflict for property %s : %s (default val) != %s (set val)", dk, dv, valInPropMap)); + } Review Comment: Same comment here about changing from IllegalStateException to AccumuloException ########## core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java: ########## @@ -273,4 +294,169 @@ private static Class<SortedKeyValueIterator<Key,Value>> loadClass(boolean useAcc log.trace("Iterator class {} loaded from classpath", iterInfo.className); return clazz; } + + public static void checkIteratorConflicts(Map<String,String> props, String property, String value) + throws AccumuloException, TableNotFoundException, IllegalArgumentException { + if (props.containsKey(property) && props.get(property).equals(value)) { + // setting a property that already exists (i.e., no change) + return; + } + if (IteratorConfigUtil.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]); + TableOperationsHelper.checkIteratorConflicts(props, givenIter, EnumSet.of(scope)); + } + } + } + + 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 (IteratorConfigUtil.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]); + noh.checkIteratorConflicts(namespace, givenIter, EnumSet.of(scope)); + } + } + + // 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<IteratorScope> scopes) + throws AccumuloException { + var tablesInNamespace = tableOps.list().stream() + .filter(t -> t.startsWith(namespace + Namespace.SEPARATOR)).collect(Collectors.toSet()); + try { + for (var table : tablesInNamespace) { + IteratorConfigUtil.checkIteratorConflicts(tableOps.getTableProperties(table), is, scopes); + } + } catch (TableNotFoundException | IllegalArgumentException e) { + throw new AccumuloException(e); + } + } + + public static void checkIteratorConflictsWithTablesInNamespace(TableOperations tableOps, + String namespace, String property, String value) throws AccumuloException { + var tablesInNamespace = tableOps.list().stream() + .filter(t -> t.startsWith(namespace + Namespace.SEPARATOR)).collect(Collectors.toSet()); + try { + for (var table : tablesInNamespace) { + IteratorConfigUtil.checkIteratorConflicts(tableOps.getTableProperties(table), property, + value); + } + } catch (TableNotFoundException | IllegalArgumentException e) { + throw new AccumuloException(e); + } + } + + public static void checkIteratorConflicts(Map<String,String> props, IteratorSetting setting, + EnumSet<IteratorScope> scopes) throws AccumuloException { + 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); + String valStr = String.format("%s,%s", setting.getPriority(), setting.getIteratorClass()); + Map<String,String> optionConflicts = new TreeMap<>(); + // skip if the setting is present in the map... not a conflict if exactly the same + if (props.containsKey(nameStr) && props.get(nameStr).equals(valStr) + && IteratorConfigUtil.containsSameIterOpts(props, setting, optStr)) { + continue; + } Review Comment: for help in review: this method is the same as before except the addition of "valStr" and this if check. Moved here since same code was used for TableOperationsHelper.checkIteratorConflicts and NamespaceOperationsHelper.checkIteratorConflicts. ########## core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java: ########## @@ -199,29 +215,51 @@ public Map<String,String> getProperties() { // 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 AccumuloException(String.format( "conflict with default table iterator: scopes: %s setting: %s", scopes, setting), e); } Review Comment: Minor mistake when I had originally written this to throw IllegalStateException. Changed to AccumuloException. This is called exclusively by TableOperations.create(), so I thought AccumuloException would be a more appropriate exception for this. This does change a public API method signature, so this may not be desired. If that is the case, I can change this to IllegalArgumentException, which is probably more appropriate than IllegalStateException ########## core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java: ########## @@ -199,29 +215,51 @@ public Map<String,String> getProperties() { // 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); Review Comment: I could remove noDefaultsPropMap since I pushed the check for equality into checkIteratorConflicts ########## core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java: ########## @@ -73,8 +72,25 @@ public class NewTableConfiguration { private Map<String,String> summarizerProps = Collections.emptyMap(); private Map<String,String> localityProps = Collections.emptyMap(); private final Map<String,String> iteratorProps = new HashMap<>(); + private final Map<String,String> inheritedIteratorProps = new HashMap<>(); private SortedSet<Text> splitProps = Collections.emptySortedSet(); + /** + * Configures the {@link NewTableConfiguration} with iterators inherited from the parent + * namespace. This is used internally in table creation - no need to call directly. + * + * @param props the parent namespace config + */ + public void configureInheritedIteratorProps(Map<String,String> props) { Review Comment: this had to be public to be accessible in TableOperationsImpl... I think this is fine since a similar thing is done for getProperties(), but let me know if this addition to the public API for 2.1 is a problem. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
