kevinrr888 commented on code in PR #6040:
URL: https://github.com/apache/accumulo/pull/6040#discussion_r2714507348
##########
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?
This method call will only log and not throw an AccumuloException (note
`false` param). It is impossible for this to throw an AccumuloException, so I
could change `IllegalArgumentException` to an assertion error or something to
make this more clear that it can't happen.
This is not the case for all conflict check methods in `IteratorConfigUtil`,
as some perform table ops, namespace ops, etc. so they can still throw an
AccumuloException, but this specific method cannot throw an AccumuloException
when `false` is provided. I considered writing different methods with different
signatures (e.g., one that "throws AccumuloException" and one that does not),
but that made things even more bloated in `IteratorConfigUtil`, and this
approach couldn't be applied to all conflict check methods (since as I
mentioned some still needed to throw an AccumuloException for other reasons).
> If we do warn does that mean this code is untestable in 2.1?
I'm working on testing the warnings via checking the logs (working on
changing `IteratorConflictsIT`), which is proving to be difficult, but I think
it's possible
--
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]
