Fix #837: worker_verbose flag has no effect.

Due to reusing an old Reporter instead of grabbing the current one for each build, verbose messages were lost and not printed to the console. Also adds a test for this feature.

--
MOS_MIGRATED_REVID=122639383

3 files changed, 26 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
index 5deafce1f4..3c444a2577 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
@@ -25,11 +25,12 @@ import org.apache.commons.pool2.impl.DefaultPooledObject;
  * Factory used by the pool to create / destroy / validate worker processes.
  */
 final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker> {
-  private Path logDir;
+  private final Path logDir;
   private Reporter reporter;
   private boolean verbose;
 
-  public void setLogDirectory(Path logDir) {
+  public WorkerFactory(Path logDir) {
+    super();
     this.logDir = logDir;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
index 5e77542085..e16bd383c2 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
@@ -37,7 +37,8 @@ import java.io.IOException;
  * A module that adds the WorkerActionContextProvider to the available action context providers.
  */
 public class WorkerModule extends BlazeModule {
-  private WorkerPool workers;
+  private WorkerFactory workerFactory;
+  private WorkerPool workerPool;
 
   private CommandEnvironment env;
   private BuildRequest buildRequest;
@@ -55,7 +56,7 @@ public class WorkerModule extends BlazeModule {
     this.env = env;
     env.getEventBus().register(this);
 
-    if (workers == null) {
+    if (workerFactory == null) {
       Path logDir = env.getOutputBase().getRelative("worker-logs");
       try {
         logDir.createDirectory();
@@ -65,6 +66,12 @@ public class WorkerModule extends BlazeModule {
             .handle(Event.error("Could not create directory for worker logs: " + logDir));
       }
 
+      workerFactory = new WorkerFactory(logDir);
+    }
+
+    workerFactory.setReporter(env.getReporter());
+
+    if (workerPool == null) {
       GenericKeyedObjectPoolConfig config = new GenericKeyedObjectPoolConfig();
 
       // It's better to re-use a worker as often as possible and keep it hot, in order to profit
@@ -85,23 +92,21 @@ public class WorkerModule extends BlazeModule {
       // worker.
       config.setMaxTotal(-1);
 
-      workers = new WorkerPool(new WorkerFactory(), config);
-      workers.setReporter(env.getReporter());
-      workers.setLogDirectory(logDir);
+      workerPool = new WorkerPool(workerFactory, config);
     }
   }
 
   @Subscribe
   public void buildStarting(BuildStartingEvent event) {
-    Preconditions.checkNotNull(workers);
+    Preconditions.checkNotNull(workerPool);
 
     this.buildRequest = event.getRequest();
 
     WorkerOptions options = buildRequest.getOptions(WorkerOptions.class);
-    workers.setMaxTotalPerKey(options.workerMaxInstances);
-    workers.setMaxIdlePerKey(options.workerMaxInstances);
-    workers.setMinIdlePerKey(options.workerMaxInstances);
-    workers.setVerbose(options.workerVerbose);
+    workerPool.setMaxTotalPerKey(options.workerMaxInstances);
+    workerPool.setMaxIdlePerKey(options.workerMaxInstances);
+    workerPool.setMinIdlePerKey(options.workerMaxInstances);
+    workerFactory.setVerbose(options.workerVerbose);
     this.verbose = options.workerVerbose;
   }
 
@@ -109,10 +114,10 @@ public class WorkerModule extends BlazeModule {
   public Iterable<ActionContextProvider> getActionContextProviders() {
     Preconditions.checkNotNull(env);
     Preconditions.checkNotNull(buildRequest);
-    Preconditions.checkNotNull(workers);
+    Preconditions.checkNotNull(workerPool);
 
     return ImmutableList.<ActionContextProvider>of(
-        new WorkerActionContextProvider(env, buildRequest, workers));
+        new WorkerActionContextProvider(env, buildRequest, workerPool));
   }
 
   @Override
@@ -122,7 +127,7 @@ public class WorkerModule extends BlazeModule {
 
   @Subscribe
   public void buildComplete(BuildCompleteEvent event) {
-    if (workers != null && buildRequest != null
+    if (workerPool != null && buildRequest != null
         && buildRequest.getOptions(WorkerOptions.class) != null
         && buildRequest.getOptions(WorkerOptions.class).workerQuitAfterBuild) {
       if (verbose) {
@@ -130,8 +135,8 @@ public class WorkerModule extends BlazeModule {
             .getReporter()
             .handle(Event.info("Build completed, shutting down worker pool..."));
       }
-      workers.close();
-      workers = null;
+      workerPool.close();
+      workerPool = null;
     }
   }
 
@@ -140,14 +145,14 @@ public class WorkerModule extends BlazeModule {
   // for them to finish.
   @Subscribe
   public void buildInterrupted(BuildInterruptedEvent event) {
-    if (workers != null) {
+    if (workerPool != null) {
       if (verbose) {
         env
             .getReporter()
             .handle(Event.info("Build interrupted, shutting down worker pool..."));
       }
-      workers.close();
-      workers = null;
+      workerPool.close();
+      workerPool = null;
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java
index bf5e458a23..d1eb29e93c 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java
@@ -14,8 +14,6 @@
 package com.google.devtools.build.lib.worker;
 
 import com.google.common.base.Throwables;
-import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.vfs.Path;
 
 import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
 import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
@@ -32,23 +30,9 @@ import javax.annotation.concurrent.ThreadSafe;
  */
 @ThreadSafe
 final class WorkerPool extends GenericKeyedObjectPool<WorkerKey, Worker> {
-  final WorkerFactory workerFactory;
 
   public WorkerPool(WorkerFactory factory, GenericKeyedObjectPoolConfig config) {
     super(factory, config);
-    this.workerFactory = factory;
-  }
-
-  public void setLogDirectory(Path logDir) {
-    this.workerFactory.setLogDirectory(logDir);
-  }
-
-  public void setReporter(Reporter reporter) {
-    this.workerFactory.setReporter(reporter);
-  }
-
-  public void setVerbose(boolean verbose) {
-    this.workerFactory.setVerbose(verbose);
   }
 
   @Override