keith-turner commented on code in PR #6040:
URL: https://github.com/apache/accumulo/pull/6040#discussion_r2714352564
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -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)) {
Review Comment:
Do you know if this code was duplicated in NamespaceOperationsHelper?
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java:
##########
@@ -57,6 +66,19 @@ public class IteratorConfigUtil {
public static final Comparator<IterInfo> ITER_INFO_COMPARATOR =
Comparator.comparingInt(IterInfo::getPriority);
+ private static final String ITERATOR_PROP_REGEX =
+ ("^" + Property.TABLE_ITERATOR_PREFIX.getKey() + "(" +
Arrays.stream(IteratorScope.values())
Review Comment:
The way these regexes are used for validation they ignore properties with
the iterator prefix that do not have a valid scope. Like maybe
`table.iterator.sacn.fooFilter` would be completely ignored and would not
flagged as a problem. Seems like the old validation code had this same
problem, so this is not a new behavior AFAICT. This should probably be a
follow on issue to look for invalid scopes following the iterator prefix.
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java:
##########
@@ -273,4 +295,216 @@ 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 {
+ if (props.containsKey(property) && props.get(property).equals(value)) {
Review Comment:
Maybe could do the following to avoid two map lookups. Its not exactly the
same though as what is there, it has slightly different behavior around nulls,
so not sure if you want that.
```suggestion
if (Objects.equals(props.get(property), value)) {
```
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java:
##########
@@ -273,4 +295,216 @@ 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 {
+ 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<IteratorScope> scopes)
+ throws AccumuloException {
+ Set<String> 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<String> 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<IteratorScope> iterScopesToCheck,
+ Map<IteratorScope,List<IteratorSetting>> 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<String,String> props,
IteratorSetting iterToCheck,
Review Comment:
Reviewing this code and the exsiting code in this area, noticed there are
multiple places that parse and serialize iter config. This code is adding to
that existing pattern. Was wondering if we could avoid this for the new code
and experimented to see what was possible. Came up w/ the following. Its very
clunky, but avoids duplicating code to parse iterator config. Feel free to
ignore this, was just trying to understand what we could do. Can do things in
follow on issues, would like to narrow the set of changes for this rather than
widen them.
```java
public static List<IteratorSetting>
propertiesToIteratorSettings(IteratorScope scope,AccumuloConfiguration config,
Consumer<Map<String, Map<String, String>>> extraOptsConsumer){
Map<String, Map<String, String>> allOptions = new HashMap<>();
List<IterInfo> info = parseIterConf(scope, List.of(), allOptions,
config);
List<IteratorSetting> iterSettings = new ArrayList<>(info.size());
info.forEach(iterInfo -> {
var options = allOptions.remove(iterInfo.getIterName());
var iterSetting =new IteratorSetting(iterInfo.getPriority(),
iterInfo.getIterName(), iterInfo.getClassName());
if(options != null){
iterSetting.addOptions(options);
}
iterSettings.add(iterSetting);
});
if(!allOptions.isEmpty()) {
// these are itertor options w/ no iterator definition
extraOptsConsumer.accept(allOptions);
}
return iterSettings;
}
public static void checkIteratorConflicts(Map<String,String> props,
IteratorSetting iterToCheck,
EnumSet<IteratorScope> iterScopesToCheck) throws AccumuloException {
// parse the props map
Map<IteratorScope,List<IteratorSetting>> existingIters =
new HashMap<>(IteratorScope.values().length);
for(var scope : iterScopesToCheck) {
var iterSettings = propertiesToIteratorSettings(scope, new
ConfigurationCopy(props), extraOpts->{
// TODO this used throw an exception w/ the first extra option it
saw,not its all extra so could be a much larger message which may cause
problems for logging
String msg = String.format("iterator options conflict for %s : %s",
iterToCheck.getName(), extraOpts);
throw new AccumuloException(new IllegalArgumentException(msg));
});
existingIters.put(scope,iterSettings);
}
// check if the given iterator conflicts with any existing iterators
checkIteratorConflicts(iterToCheck, iterScopesToCheck, existingIters);
}
```
This would be a follow on issue, but it would be nice to consolidate
parsing/serializing in the existing code. Maybe its better address this
comprehensively in a PR focused on this single problem rather than partially
here for only new code, not sure.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java:
##########
@@ -243,6 +246,32 @@ private SortedKeyValueIterator<Key,Value> 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<IteratorSetting> 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) {
Review Comment:
Nice how this avoid creating and populating this list unless its needed.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java:
##########
@@ -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());
- }
Review Comment:
> it is quietly ignored as not being an iterator-related property.
We should not quietly ignore invalid properties under the iterator prefix,
this could be a sign of a typo. If its a typo then the user could expect
something to be working and it silently does not. Would be good to fail or
warn for that. Made a comment elsewhere about the scope, seems this new code
and the old code ignores invalid scopes. Not completely sure about this this
though.
##########
server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java:
##########
@@ -322,22 +323,27 @@ public void executeFateOperation(TInfo tinfo,
TCredentials c, long opid, FateOpe
Map<String,String> propertiesToSet = new HashMap<>();
Set<String> 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 =
Review Comment:
Saw two problems w/ this code. First when it subtract namespace config
that could remove props that are in both src table and src namepsace. Second
it does not remove props to exclude. For the first problem we can get only the
src table props, not the merged view. Tried to fix this issues below.
```java
// dest table will have the dest namespace props + src table props:
need to check provided
// options to set for conflicts with this
var iterProps = new
HashMap<>(manager.getContext().getNamespaceConfiguration(namespaceId)
.getAllPropertiesWithPrefix(Property.TABLE_ITERATOR_PREFIX));
// get only the source table props, not the merged view
var srcTableProps =
manager.getContext().getPropStore().get(TablePropKey.of(manager.getContext(),
srcTableId)).asMap();
for (Entry<String,String> entry : options.entrySet()) {
if
(entry.getKey().startsWith(TableOperationsImpl.PROPERTY_EXCLUDE_PREFIX)) {
propertiesToExclude.add(
entry.getKey().substring(TableOperationsImpl.PROPERTY_EXCLUDE_PREFIX.length()));
}
}
// these props will not be cloned
srcTableProps.keySet().removeAll(propertiesToExclude);
//merge src table props into dest namespace props
iterProps.putAll(srcTableProps);
for (Entry<String,String> entry : options.entrySet()) {
if
(entry.getKey().startsWith(TableOperationsImpl.PROPERTY_EXCLUDE_PREFIX)) {
continue;
}
validateTableProperty(entry.getKey(), entry.getValue(), options,
IteratorConfigUtil.gatherIteratorProps(iterProps), tableName,
tableOp);
propertiesToSet.put(entry.getKey(), entry.getValue());
}
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java:
##########
@@ -243,6 +246,32 @@ private SortedKeyValueIterator<Key,Value> 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<IteratorSetting> 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);
Review Comment:
Will this fail the scan? If so should this warn? If we do warn does that
mean this code is untestable in 2.1?
--
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]
