orders iterators by name when priority the same (#6075)

Author: keith-turner

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

@@ -55,7 +55,8 @@ public class IteratorConfigUtil {
   private static final Logger log = LoggerFactory.getLogger(IteratorConfigUtil.class);
 
   public static final Comparator<IterInfo> ITER_INFO_COMPARATOR =
-      Comparator.comparingInt(IterInfo::getPriority);
+      Comparator.comparingInt(IterInfo::getPriority)
+          .thenComparing(iterInfo -> iterInfo.getIterName() == null ? "" : iterInfo.getIterName());
 
   /**
    * Fetch the correct configuration key prefix for the given scope. Throws an

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

@@ -103,6 +103,7 @@
 import org.apache.accumulo.test.VerifyIngest.VerifyParams;
 import org.apache.accumulo.test.compaction.CompactionExecutorIT;
 import org.apache.accumulo.test.compaction.ExternalCompaction_1_IT.FSelector;
+import org.apache.accumulo.test.functional.ScanIteratorIT.AppendingIterator;
 import org.apache.accumulo.test.util.Wait;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileSystem;
@@ -988,6 +989,56 @@ public void testMigrationCancelCompaction() throws Exception {
     }
   }
 
+  @Test
+  public void testIteratorOrder() throws Exception {
+    String[] names = getUniqueNames(2);
+    try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) {
+
+      // create a table with minor compaction iterators configured to ensure those iterators are
+      // applied in the correct order
+      NewTableConfiguration ntc = new NewTableConfiguration()
+          .attachIterator(AppendingIterator.configure(50, "x"), EnumSet.of(IteratorScope.minc))
+          .attachIterator(AppendingIterator.configure(100, "a"), EnumSet.of(IteratorScope.minc));
+      c.tableOperations().create(names[0], ntc);
+
+      // create a table with major compaction iterators configured to ensure those iterators are
+      // applied in the correct order
+      NewTableConfiguration ntc2 = new NewTableConfiguration()
+          .attachIterator(AppendingIterator.configure(50, "x"), EnumSet.of(IteratorScope.majc))
+          .attachIterator(AppendingIterator.configure(100, "a"), EnumSet.of(IteratorScope.majc));
+      c.tableOperations().create(names[1], ntc2);
+
+      try (var writer = c.createBatchWriter(names[0]);
+          var writer2 = c.createBatchWriter(names[1])) {
+        Mutation m = new Mutation("r1");
+        m.put("", "", "base:");
+        writer.addMutation(m);
+        writer2.addMutation(m);
+      }
+
+      try (var mincScanner = c.createScanner(names[0]);
+          var majcScanner = c.createScanner(names[1])) {
+        // iterators should not be applied yet
+        assertEquals("base:", mincScanner.iterator().next().getValue().toString());
+        assertEquals("base:", majcScanner.iterator().next().getValue().toString());
+
+        c.tableOperations().flush(names[0], null, null, true);
+        assertEquals("base:xa", mincScanner.iterator().next().getValue().toString());
+        assertEquals("base:", majcScanner.iterator().next().getValue().toString());
+
+        // The user compaction iterators with priority 50 and 100 have the same priority as table
+        // level iterators.
+        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());
+
+      }
+    }
+  }
+
   /**
    * Counts the number of tablets and files in a table.
    */

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

@@ -21,12 +21,15 @@
 import static org.apache.accumulo.harness.AccumuloITBase.SUNNY_DAY;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import java.io.IOException;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 
 import org.apache.accumulo.cluster.ClusterUser;
 import org.apache.accumulo.core.client.Accumulo;
@@ -38,11 +41,17 @@
 import org.apache.accumulo.core.client.Scanner;
 import org.apache.accumulo.core.client.ScannerBase;
 import org.apache.accumulo.core.client.TableNotFoundException;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
+import org.apache.accumulo.core.client.admin.NewTableConfiguration;
 import org.apache.accumulo.core.client.security.tokens.PasswordToken;
 import org.apache.accumulo.core.data.Key;
 import org.apache.accumulo.core.data.Mutation;
 import org.apache.accumulo.core.data.Range;
 import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.IteratorUtil.IteratorScope;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.iterators.WrappingIterator;
 import org.apache.accumulo.core.security.Authorizations;
 import org.apache.accumulo.core.security.TablePermission;
 import org.apache.accumulo.harness.AccumuloClusterHarness;
@@ -250,4 +259,62 @@ private void writeTestMutation(AccumuloClient userC)
       batchWriter.flush();
     }
   }
+
+  public static class AppendingIterator extends WrappingIterator {
+
+    private String append;
+
+    @Override
+    public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options,
+        IteratorEnvironment env) throws IOException {
+      super.init(source, options, env);
+      append = Objects.requireNonNull(options.get("append"));
+    }
+
+    @Override
+    public Value getTopValue() {
+      var val = super.getTopValue();
+      return new Value(val.toString() + append);
+    }
+
+    public static IteratorSetting configure(int prio, String append) {
+      IteratorSetting iter = new IteratorSetting(prio, "append_" + append, AppendingIterator.class);
+      iter.addOption("append", append);
+      return iter;
+    }
+  }
+
+  // Ensures scan iterators run in the expected order
+  @Test
+  public void testIteratorOrder() throws Exception {
+
+    String tableName = getUniqueNames(2)[1];
+    try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) {
+      var scopes = EnumSet.of(IteratorScope.scan);
+      NewTableConfiguration ntc =
+          new NewTableConfiguration().attachIterator(AppendingIterator.configure(50, "x"), scopes)
+              .attachIterator(AppendingIterator.configure(100, "a"), scopes);
+      c.tableOperations().create(tableName, ntc);
+
+      try (var writer = c.createBatchWriter(tableName)) {
+        Mutation m = new Mutation("r1");
+        m.put("", "", "base:");
+        writer.addMutation(m);
+      }
+
+      try (var scanner = c.createScanner(tableName)) {
+        assertEquals("base:xa", scanner.iterator().next().getValue().toString());
+        scanner.addScanIterator(AppendingIterator.configure(70, "m"));
+        assertEquals("base:xma", scanner.iterator().next().getValue().toString());
+        // These have the same priority as a table iterator so the iterators should be ordered by
+        // their name in that case
+        scanner.addScanIterator(AppendingIterator.configure(50, "b"));
+        scanner.addScanIterator(AppendingIterator.configure(100, "c"));
+        assertEquals("base:bxmac", scanner.iterator().next().getValue().toString());
+        // 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());
+      }
+    }
+  }
 }