Prevents iterator conflicts #6040

kevinrr888 · 2026-01-07T19:38:45Z

This adds checks when adding an iterator that the given iterator does not conflict with any existing iterators. Conflict meaning same name or same priority. Iterators can be added several ways, and previously only TableOperations.attachIterator and NamespaceOperations.attachIterator would check for conflicts. This adds iterator conflict checks to:

Scanners at the time they are used
TableOperations.setProperty
TableOperations.modifyProperties
NewTableConfiguration.attachIterator
NamespaceOperations.attachIterator (was previously only checking for conflicts with iterators in the namespace, now also checks for conflicts with iterators in the tables of the namespace)
NamespaceOperations.setProperty
NamespaceOperations.modifyProperties
CloneConfiguration.Builder.setPropertiesToSet

This also accounts for the several ways in which conflicts can arise:

Iterators that are attached directly to a table (either through TableOperations.attachIterator, TableOperations.setProperty, or TableOperations.modifyProperties)
Iterators that are attached to a namespace, inherited by a table (either through NamespaceOperations.attachIterator, NamespaceOperations.setProperty, or NamespaceOperations.modifyProperties)
Conflicts with default table iterators (if the table has them)
Adding the exact iterator already present should not fail

This commit also adds a new IteratorConflictsIT to test all of the above.

Part of #6030

This commit adds checks when adding an iterator that the given iterator does not conflict with any existing iterators. Conflict meaning same name or same priority. Iterators can be added several ways, and previously only TableOperations.attachIterator and NamespaceOperations.attachIterator would check for conflicts. This commit adds iterator conflict checks to: - Scanner.addScanIterator - TableOperations.setProperty - TableOperations.modifyProperties - NewTableConfiguration.attachIterator Note that this does not add conflict checks to NamespaceOperations.setProperty or NamespaceOperations.modifyProperties, these will be done in another commit. This commit also accounts for the several ways in which conflicts can arise: - Iterators that are attached directly to a table (either through TableOperations.attachIterator, TableOperations.setProperty, or TableOperations.modifyProperties) - Iterators that are attached to a namespace, inherited by a table (either through NamespaceOperations.attachIterator, NamespaceOperations.setProperty, or NamespaceOperations.modifyProperties) - Conflicts with default table iterators (if the table has them) - Adding the exact iterator already present should not fail This commit also adds a new IteratorConflictsIT to test all of the above. Part of apache#6030

Adds conflict checks to: - NamespaceOperations.attachIterator (was previously only checking for conflicts with iterators in the namespace, now also checks for conflicts with iterators in the tables of the namespace) - NamespaceOperations.setProperty (check conflicts with namespace iterators and all tables in the namespace) - NamespaceOperations.modifyProperties (check conflicts with namespace iterators and all tables in the namespace) New tests to IteratorConflictsIT to test the above

kevinrr888

From running sunny day tests and all the tests I have changed in this PR, noticed that I unknowingly added new permission requirements to at least TableOperations.create() (new required permission ALTER_NAMESPACE) and Scanner.addScanIterator() (new required permission ALTER_TABLE). I imagine this is a blocker for these changes at this point, but let me know if it's not. I'll look into an alternative to avoid these permissions. See changes to ConditionalWriterIT, ScanIteratorIT, and ShellServerIT for examples of the failures I encountered.

Checks are now done server side as of cb2eccb, avoiding these permission requirements.

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

test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java

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

core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java

kevinrr888 · 2026-01-08T15:53:35Z

Transferring to WIP until I resolve #6040 (review)
Edit: Addressed

core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java

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

ctubbsii · 2026-01-08T22:48:54Z

Discussed iterator conflicts today, and here's a summary of some key points:

Conflict within the config: In configuration, no two iterators at the same scope (scan, minc, majc) may be able to have the same priority.
- This applies only to the complete view of the TableConfiguration, with all inherited properties from parent configs (namespace, system, ...), so it is okay, for example, if a table config set at the namespace level is overridden in part at the table level, so that the one single iterator at that scope and priority has configuration that spans across two levels of the configs. What is important is that the resulting view of the TableConfiguration when trying to construct an iterator stack, will not show any two different iterators at the same scope with the same priority.
- Checks could be in place when editing table/namespace configuration to ensure a priority isn't "doubled up". A user who wishes to replace iteratorA with iteratorB at the same priority would have to remove iteratorA before adding iteratorB, or would have to use modifyProperties to atomically mutate the properties to remove and add at the same time, in order to avoid an error. This alone, however, does not guarantee that there isn't a conflict. If iteratorA had been set at the namespaceN.tableT, but iteratorB was being added to namespaceN, we would have to check that there isn't a conflict with any of the tables in namespaceN. That's not exactly practical, so we may just want to check that there isn't a conflict at the level being modified, and rely on later checks when setting up the iterator stack to verify that there isn't a conflict overall.
- Note: this probably would be easier to deconflict if our iterator configs used a different property key scheme that was more overrideable atomically, like table.<scope>.iterator.<priority>=class,opt1key=opt1val,opt2key=opt2val,.... so that it wouldn't be possible to have conflict between namespace and table configs, because one would fully override the other. But, that's not what we have today.
Conflict in user-supplied iterators for a specific scan/compaction: No two iterators in a single client-initiated operation's settings may have the same priority.
- This is for things like scan-time iterators set in the API for a scan, and passed over the RPC, rather than iterators set in configuration on the server-side.
- This also applies to any other place where we might be able to specify iterators that aren't in the configuration (compactions, conditional mutations, etc.)
- We could check for conflicts set on the scanner easily, but would have to rely on the server-side setting up the iterator stack to ensure no conflicts between the user iterators and those set on the table.
Conflict between configured iterators and user-supplied iterators: The complete iterator stack for an operation may not have any iterators running at the same priority, regardless of whether it came from the configuration or from the client API/RPC request.
- To address this, we can simply check the full iterator stack when it is being constructed on the server-side, and fail the operation if any priorities are reused, regardless of where they came from.
- Alternatively, we could treat one as overriding another, but I don't think that's a very good idea.
- As a follow-on improvement here, we could treat all configured iterators as higher priority than all client operation-specific (scan-time/compaction-time) iterators:
  1. Instead of ordering three configured iterators and two user-supplied iterators by priority alone, as in C1, U2, C3, U4, C5, we would instead order them as C1, C3, C5, U2, U4.
  2. This enables stronger security guarantees by preventing a user-supplied iterator from seeing data that is filtered out in a administrator-configured iterator.
  3. This prevents bugs that could be caused by a user-supplied iterator that transforms data in a way that a subsequent administrator-configured iterator won't be able to handle.
  4. This is a behavior change, and may break some people's (ill-advised) uses, but I think it is better overall.
  5. This would also open the possibility of having a cleaner client-side API, because you don't actually have to specify priority numbers on the client. Instead, clients only need to order user-supplied iterators with respect to other user-supplied iterators, and won't need a priority number to indicate a global ordering that includes the configured iterators for a table. So, we could have an API something like: scanner.map(iterator1).map(iterator2).map(iterator3).scan().

- Moves the iterator conflict check for create table from client side to server side. - Checking if iterators added to scanner conflict with those already set on the table moved from client side to server side. - Adds iterator conflict checks to CloneConfiguration.Builder.setPropertiesToSet. This check is done server side. - Adds testing to IteratorConflictsIT for CloneConfiguration.Builder.setPropertiesToSet

test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java

server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java

keith-turner · 2026-01-14T01:21:30Z

@kevinrr888 why did you choose to do this work in 2.1 instead of in main? Seems there is chance if introducing new bugs in scans or compactions. Also may make config that used to work stop working (that is probably a good thing overall as it can help detect existing problems, but could introduce temporary pain). I am not opposed to making this change in 2.1, but was just curious.

kevinrr888 · 2026-01-14T15:31:23Z

@kevinrr888 why did you choose to do this work in 2.1 instead of in main? Seems there is chance if introducing new bugs in scans or compactions. Also may make config that used to work stop working (that is probably a good thing overall as it can help detect existing problems, but could introduce temporary pain). I am not opposed to making this change in 2.1, but was just curious.

@keith-turner I had already started this work in 2.1 with #5990 thinking this was a one-off issue with NewTableConfiguration. I did not anticipate follow on work requiring changes in as many areas, so continued with 2.1 learning the scope of the issue as I went. I also thought this validation would be good to have in the earliest version possible since it is essentially a bug. I would be fine refactoring this for main if we think this is too risky or undesired for 2.1.

keith-turner · 2026-01-14T17:11:34Z

I would be fine refactoring this for main if we think this is too risky or undesired for 2.1.

There are benefits and risk with this change. Maybe the best way to get the benefits and lower the risk is to make these changes only warn in 2.1 and fail in 4.0? That way things that were working in 2.1.4 and earlier do not blow up in 2.1.5, but still work and get a warning that iterator config is not correct and could lead to non-deterministic behavior.

kevinrr888 · 2026-01-14T17:35:37Z

make these changes only warn in 2.1 and fail in 4.0

This is good with me, I'll change this

Also fixed a bug where I was calling regex.matches(str) instead of str.matches(regex)

core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java

Iterator option conflicts were not considered in new impl of checkIteratorConflicts

Co-authored-by: Keith Turner <kturner@apache.org>

…owing

kevinrr888 · 2026-01-21T16:35:03Z

I pushed 469a83e to have the newly added iterator conflict checks only log instead of throw in 2.1. In merging to main, all checks will throw.
Still need to update the new IteratorConflictsIT, which currently expects these checks to throw

keith-turner

Still looking at this. Feel free to ignore some of the comments, can be follow on issues. I keep noticing existing problems when looking at this.

server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java

keith-turner · 2026-01-21T17:23:28Z

server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java

+                  EnumSet.of(IteratorScope.scan), Map.of(IteratorScope.scan, picIteratorSettings),
+                  false);
+            } catch (AccumuloException e) {
+              throw new IllegalArgumentException(e);


Will this fail the scan? If so should this warn? If we do warn does that mean this code is untestable in 2.1?

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

core/src/main/java/org/apache/accumulo/core/iteratorsImpl/IteratorConfigUtil.java

core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java

server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java

core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java

…o logging warning

- Avoid two look up for some comparisons - Fix some bugs with the iterator conflict checks done on clone table Co-authored-by: Keith Turner <kturner@apache.org>

kevinrr888 · 2026-01-23T15:06:30Z

@keith-turner - I believe I addressed and/or responded to all non-follow on issues for this

keith-turner

I opened PR kevinrr888#3 against your branch. The duplicate parsing code was making it really hard for me to review this because I kept wondering if it was consistent and correct every time I saw it. So I decided to take a stab at improving it, partially addressing #6074.

test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java

…atorProperty.java Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

…atorConfigUtil.java Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

consolidated some duplicate iterator parsing code

kevinrr888 · 2026-01-26T21:07:59Z

Note-to-self/additional context: See #6075 (comment) before merging

kevinrr888 added 2 commits January 6, 2026 15:59

kevinrr888 added this to the 2.1.5 milestone Jan 7, 2026

kevinrr888 self-assigned this Jan 7, 2026

kevinrr888 commented Jan 7, 2026

View reviewed changes

kevinrr888 added 2 commits January 7, 2026 16:13

Merge branch '2.1' into 2.1-feature-6030

32ccd5a

fix build

1e04009

kevinrr888 marked this pull request as draft January 8, 2026 15:53

keith-turner reviewed Jan 8, 2026

View reviewed changes

kevinrr888 mentioned this pull request Jan 9, 2026

Fix exception bug with executeFateOperation #6048

Merged

kevinrr888 marked this pull request as ready for review January 13, 2026 18:49

kevinrr888 added 2 commits January 13, 2026 14:03

minor fix and cleanup

d518c3c

trivial

b0c0644

kevinrr888 commented Jan 13, 2026

View reviewed changes

test/src/main/java/org/apache/accumulo/test/functional/IteratorConflictsIT.java Outdated Show resolved Hide resolved

keith-turner reviewed Jan 14, 2026

View reviewed changes

server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java Show resolved Hide resolved

keith-turner mentioned this pull request Jan 14, 2026

Investigate changing scope of server side iterator resolution #6055

Open

Improved efficiency of iterator conflict check at scan time

f422862

Also fixed a bug where I was calling regex.matches(str) instead of str.matches(regex)

keith-turner reviewed Jan 16, 2026

View reviewed changes

core/src/main/java/org/apache/accumulo/core/clientImpl/NamespaceOperationsHelper.java Show resolved Hide resolved

kevinrr888 and others added 4 commits January 16, 2026 16:50

Fix bug:

6a3f9f8

Iterator option conflicts were not considered in new impl of checkIteratorConflicts

Merge branch '2.1' into 2.1-feature-6030

539e082

Implemented more efficient alg for iterator conflict checks at scan time

2e5da9b

Co-authored-by: Keith Turner <kturner@apache.org>

Newly added iterator conflict checks now log a warning instead of thr…

469a83e

…owing

Merge branch '2.1' into 2.1-feature-6030

f79f573

keith-turner reviewed Jan 21, 2026

View reviewed changes

kevinrr888 and others added 3 commits January 22, 2026 14:43

Updated IteratorConflictsIT based on switch from throwing exception t…

d6e9e1c

…o logging warning

fix build failure: log4j-core and log4j-api deps added to test

8774df5

Address review:

f72d9a4

- Avoid two look up for some comparisons - Fix some bugs with the iterator conflict checks done on clone table Co-authored-by: Keith Turner <kturner@apache.org>

This was referenced Jan 23, 2026

Iterator parsing code is duplicated #6074

Open

orders iterators by name when priority the same #6075

Merged

consolidated some duplicate iterator parsing code

d3cad32

keith-turner reviewed Jan 23, 2026

View reviewed changes

keith-turner and others added 8 commits January 23, 2026 21:59

fix bug

f4925db

improve parsing

4a4e5c2

make constructors private

201318f

Update core/src/main/java/org/apache/accumulo/core/iteratorsImpl/Iter…

7ba91a2

…atorProperty.java Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

Update core/src/main/java/org/apache/accumulo/core/iteratorsImpl/Iter…

88a52b4

…atorConfigUtil.java Co-authored-by: Kevin Rathbun <kevinrr888@gmail.com>

code review update

8e4273e

Merge pull request #3 from keith-turner/2.1-feature-6030

dd891f6

consolidated some duplicate iterator parsing code

address review

3244196

keith-turner approved these changes Jan 26, 2026

View reviewed changes

Prevents iterator conflicts #6040

Are you sure you want to change the base?

Prevents iterator conflicts #6040

Uh oh!

Conversation

kevinrr888 commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinrr888 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrr888 commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctubbsii commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keith-turner commented Jan 14, 2026

Uh oh!

kevinrr888 commented Jan 14, 2026

Uh oh!

keith-turner commented Jan 14, 2026

Uh oh!

kevinrr888 commented Jan 14, 2026

Uh oh!

Uh oh!

kevinrr888 commented Jan 21, 2026

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keith-turner Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

kevinrr888 Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrr888 commented Jan 23, 2026

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrr888 commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinrr888 commented Jan 7, 2026 •

edited

Loading

kevinrr888 left a comment •

edited

Loading

kevinrr888 commented Jan 8, 2026 •

edited

Loading

ctubbsii commented Jan 8, 2026 •

edited

Loading

kevinrr888 Jan 21, 2026 •

edited

Loading