Clean up AbstractQueueVisitor's constructors.

The "concurrent" bit was supposedly around for testing purposes, but who knows if it even works anymore. Making other callsites explicitly state their ErrorClassifier gets us down to two constructors, one of which can delegate to the other.

I think having both these constructors is useful because there's a linkage between creating a new executor service and specifying that the AQV should shut down the service at the end of the visitation. And using a static create() method doesn't work because of AQV's inheritance model.

PiperOrigin-RevId: 155406771

3 files changed, 46 insertions, 27 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java b/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java
index 456b394a85..1ed396aae6 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ConcurrentMultimapWithHeadElementTest.java
@@ -20,17 +20,16 @@ import static org.junit.Assert.assertTrue;
 
 import com.google.common.testing.GcFinalization;
 import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
+import com.google.devtools.build.lib.concurrent.ErrorClassifier;
 import com.google.devtools.build.lib.testutil.TestThread;
 import com.google.devtools.build.lib.testutil.TestUtils;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.lang.ref.WeakReference;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Tests for ConcurrentMultimapWithHeadElement.
@@ -128,19 +127,20 @@ public class ConcurrentMultimapWithHeadElementTest {
     }
   }
 
-  private class StressTester extends AbstractQueueVisitor {
+  private static class StressTester extends AbstractQueueVisitor {
     private final ConcurrentMultimapWithHeadElement<Boolean, Integer> multimap =
         new ConcurrentMultimapWithHeadElement<>();
     private final AtomicInteger actionCount = new AtomicInteger(0);
 
     private StressTester() {
       super(
-          /*concurrent=*/ true,
           200,
           1,
           TimeUnit.SECONDS,
           /*failFastOnException=*/ true,
-          "action-graph-test");
+          "action-graph-test",
+          AbstractQueueVisitor.EXECUTOR_FACTORY,
+          ErrorClassifier.DEFAULT);
     }
 
     private void addAndRemove(final Boolean key, final Integer add, final Integer remove) {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java b/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
index a527549e6c..d83cf815d3 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/MapBasedActionGraphTest.java
@@ -21,18 +21,17 @@ import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictEx
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil.UncheckedActionConflictException;
 import com.google.devtools.build.lib.actions.util.TestAction;
 import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
+import com.google.devtools.build.lib.concurrent.ErrorClassifier;
 import com.google.devtools.build.lib.util.BlazeClock;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Tests for {@link MapBasedActionGraph}.
@@ -73,7 +72,7 @@ public class MapBasedActionGraphTest {
     actionGraph.unregisterAction(action);
   }
 
-  private class ActionRegisterer extends AbstractQueueVisitor {
+  private static class ActionRegisterer extends AbstractQueueVisitor {
     private final MutableActionGraph graph = new MapBasedActionGraph();
     private final Artifact output;
     // Just to occasionally add actions that were already present.
@@ -82,12 +81,13 @@ public class MapBasedActionGraphTest {
 
     private ActionRegisterer() {
       super(
-          /*concurrent=*/ true,
           200,
           1,
           TimeUnit.SECONDS,
           /*failFastOnException=*/ true,
-          "action-graph-test");
+          "action-graph-test",
+          AbstractQueueVisitor.EXECUTOR_FACTORY,
+          ErrorClassifier.DEFAULT);
       FileSystem fileSystem = new InMemoryFileSystem(BlazeClock.instance());
       Path path = fileSystem.getPath("/root/foo");
       output = new Artifact(path, Root.asDerivedRoot(path));
diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java
index 7be04c4547..5aa63dd94c 100644
--- a/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitorTest.java
@@ -462,7 +462,6 @@ public class AbstractQueueVisitorTest {
     };
     AbstractQueueVisitor visitor =
         new AbstractQueueVisitor(
-            /*concurrent=*/ true,
             executor,
             /*shutdownOnCompletion=*/ true,
             /*failFastOnException=*/ false,
@@ -557,11 +556,18 @@ public class AbstractQueueVisitorTest {
     private final Object lock = new Object();
 
     public CountingQueueVisitor() {
-      super(/*parallelism=*/ 5, /*keepAlive=*/ 3L, TimeUnit.SECONDS, THREAD_NAME);
+      super(
+          /*parallelism=*/ 5,
+          /*keepAlive=*/ 3L,
+          TimeUnit.SECONDS,
+          /*failFast=*/ false,
+          THREAD_NAME,
+          AbstractQueueVisitor.EXECUTOR_FACTORY,
+          ErrorClassifier.DEFAULT);
     }
 
-    public CountingQueueVisitor(ThreadPoolExecutor executor) {
-      super(executor, false, true);
+    CountingQueueVisitor(ThreadPoolExecutor executor) {
+      super(executor, false, true, ErrorClassifier.DEFAULT);
     }
 
     public void enqueue() {
@@ -588,23 +594,36 @@ public class AbstractQueueVisitorTest {
 
     private final static String THREAD_NAME = "BlazeTest ConcreteQueueVisitor";
 
-    public ConcreteQueueVisitor() {
-      super(5, 3L, TimeUnit.SECONDS, THREAD_NAME);
+    ConcreteQueueVisitor() {
+      super(
+          5,
+          3L,
+          TimeUnit.SECONDS,
+          /*failFast=*/ false,
+          THREAD_NAME,
+          AbstractQueueVisitor.EXECUTOR_FACTORY,
+          ErrorClassifier.DEFAULT);
     }
 
-    public ConcreteQueueVisitor(boolean failFast) {
-      super(true, 5, 3L, TimeUnit.SECONDS, failFast, THREAD_NAME);
+    ConcreteQueueVisitor(boolean failFast) {
+      super(
+          5,
+          3L,
+          TimeUnit.SECONDS,
+          failFast,
+          THREAD_NAME,
+          AbstractQueueVisitor.EXECUTOR_FACTORY,
+          ErrorClassifier.DEFAULT);
     }
 
-    public ConcreteQueueVisitor(ThreadPoolExecutor executor, boolean failFast) {
-      super(executor, /*shutdownOnCompletion=*/ false, failFast);
+    ConcreteQueueVisitor(ThreadPoolExecutor executor, boolean failFast) {
+      super(executor, /*shutdownOnCompletion=*/ false, failFast, ErrorClassifier.DEFAULT);
     }
   }
 
   private static AbstractQueueVisitor createQueueVisitorWithConstantErrorClassification(
       ThreadPoolExecutor executor, final ErrorClassification classification) {
     return new AbstractQueueVisitor(
-        /*concurrent=*/ true,
         executor,
         /*shutdownOnCompletion=*/ true,
         /*failFastOnException=*/ false,