orders iterators by name when priority the same#6075
orders iterators by name when priority the same#6075keith-turner merged 3 commits intoapache:2.1from
Conversation
| public static final Comparator<IterInfo> ITER_INFO_COMPARATOR = | ||
| Comparator.comparingInt(IterInfo::getPriority); | ||
| Comparator.comparingInt(IterInfo::getPriority) | ||
| .thenComparing(iterInfo -> iterInfo.getIterName() == null ? "" : iterInfo.getIterName()); |
There was a problem hiding this comment.
Do not expect the name to be null. Was being defensive w/ this null check since this a 2.1 change. Did not want something that used to work to start throwing a NPE.
| // There are no compaction iterators, so this should not change value | ||
| c.tableOperations().compact(tableName, new CompactionConfig().setWait(true)); | ||
| assertEquals("base:bxmac", scanner.iterator().next().getValue().toString()); | ||
| } |
There was a problem hiding this comment.
Should we add a test that configures two iterators at the same priority and different names, or is that here and I missed it?
There was a problem hiding this comment.
The merged iterators will have that. The table has iterators with name append_x,50 and append_a,100. The scanner sets iterators with append_b,50 and append_c,100.
Trying to make the scanner have that will throw an exception and that may be covered by other test. For this test wanted to ensure the merged view had a consistent order. Also I am not sure there is any test for the iterator order, so this test also covers that basic case.
| List<IteratorSetting> iters = List.of(AppendingIterator.configure(70, "m"), | ||
| AppendingIterator.configure(50, "b"), AppendingIterator.configure(100, "c")); | ||
| c.tableOperations().compact(names[1], | ||
| new CompactionConfig().setWait(true).setFlush(true).setIterators(iters)); | ||
| assertEquals("base:xa", mincScanner.iterator().next().getValue().toString()); | ||
| assertEquals("base:bxmac", majcScanner.iterator().next().getValue().toString()); |
There was a problem hiding this comment.
A comment to note that (50, "b") and (100, "c") share the same priority as other iterators would be helpful
| c.tableOperations().compact(names[1], | ||
| new CompactionConfig().setWait(true).setFlush(true).setIterators(iters)); |
There was a problem hiding this comment.
A few comments:
- This made me realize I did not consider
CompactionConfigin Prevents iterator conflicts #6040, so even with 6040, this call will not log a warning (should log a warning in 2.1 since we are setting a majc iter with the same priority as a majc iter set on the table). To avoid adding more to that PR, should probably open a follow on issue to address this after 6040 is merged. - I think all of the testing added in this PR will need to be removed in main when Prevents iterator conflicts #6040 is merged up to main. This is because adding an iterator of the same priority will cause an exception in main. As mentioned above, there's the
CompactionConfigcaveat for now, but this testing should still be removed with the merging of 6040 into main. The follow on issue will address makingCompactionConfigthrow in the case of attempting to add an iter of existing priority (or name) and should add testing toIteratorConflictsITto confirm it does throw. - I can keep the changes made to
ITER_INFO_COMPARATORin this PR in main for consistency
These are comments for dicussion/notes-to-self when I merge 6040, nothing to change in this PR
There was a problem hiding this comment.
I think all of the testing added in this PR will need to be removed in main when #6040 is merged up to main.
Want to keep a subset of the testing. Still want to test that iterators are applied in the expected order, can drop testing of duplicate priorities.
There was a problem hiding this comment.
Okay, that sounds good. I thought this was mostly for testing dup priorities and there was testing elsewhere for expected ordering, but maybe there is no such testing...
3 participants
This is follow on work from #6040. It modifies the sorting of iterators to consider name when the priority is equals. This makes iterator order more deterministic for this case. Also adds scan and compaction test that ensure iterators are applied in the correct order.