Address Feedback

src/main/java/build/buildfarm/worker/Executor.java

@@ -65,13 +65,10 @@
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
-import java.util.logging.Logger;
 import lombok.extern.java.Log;
 
 @Log
 class Executor {
-  private static final Logger logger = Logger.getLogger(Executor.class.getName());
-
   private static final int INCOMPLETE_EXIT_CODE = -1;
 
   private final WorkerContext workerContext;
@@ -445,7 +442,7 @@ private Code executeCommand(
         !limits.persistentWorkerKey.isEmpty() && !limits.persistentWorkerCommand.isEmpty();
 
     if (usePersistentWorker) {
-      logger.fine(
+      log.fine(
           "usePersistentWorker; got persistentWorkerCommand of : "
               + limits.persistentWorkerCommand);
 

src/main/java/build/buildfarm/worker/persistent/BUILD

@@ -1,6 +1,7 @@
 java_library(
     name = "persistent",
     srcs = glob(["*.java"]),
+    plugins = ["//src/main/java/build/buildfarm/common:lombok"],
     visibility = ["//visibility:public"],
     deps = [
         "//persistentworkers/src/main/java/persistent/bazel:bazel-persistent-workers",
@@ -25,5 +26,6 @@ java_library(
         "@maven//:io_prometheus_simpleclient",
         "@maven//:org_apache_commons_commons_compress",
         "@maven//:org_jetbrains_annotations",
+        "@maven//:org_projectlombok_lombok",
     ],
 )

src/main/java/build/buildfarm/worker/persistent/FileAccessUtils.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import static java.nio.file.StandardCopyOption.COPY_ATTRIBUTES;
@@ -11,18 +25,17 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Supplier;
-import java.util.logging.Logger;
+import lombok.extern.java.Log;
 
 /**
  * Utility for concurrent move/copy of files Can be extended in the future to (sym)linking if we
  * need performance
  */
+@Log
 public final class FileAccessUtils {
   // singleton class with only static methods
   private FileAccessUtils() {}
 
-  private static final Logger logger = Logger.getLogger(FileAccessUtils.class.getName());
-
   public static Path addPosixOwnerWrite(Path absPath) throws IOException {
     Set<PosixFilePermission> perms = Files.getPosixFilePermissions(absPath);
 
@@ -54,7 +67,7 @@ private PathLock() {}
    */
   public static void copyFile(Path from, Path to) throws IOException {
     Path absTo = to.toAbsolutePath();
-    logger.finer("copyFile: " + from + " to " + absTo);
+    log.finer("copyFile: " + from + " to " + absTo);
     if (!Files.exists(from)) {
       throw new IOException("copyFile: source file doesn't exist: " + from);
     }
@@ -86,7 +99,7 @@ public static void copyFile(Path from, Path to) throws IOException {
    */
   public static void moveFile(Path from, Path to) throws IOException {
     Path absTo = to.toAbsolutePath();
-    logger.finer("moveFile: " + from + " to " + absTo);
+    log.finer("moveFile: " + from + " to " + absTo);
     if (!Files.exists(from)) {
       throw new IOException("moveFile: source file doesn't exist: " + from);
     }

src/main/java/build/buildfarm/worker/persistent/Keymaker.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import com.google.common.collect.ImmutableList;

src/main/java/build/buildfarm/worker/persistent/PersistentExecutor.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import build.bazel.remote.execution.v2.ActionResult;
@@ -16,8 +30,8 @@
 import java.nio.file.Paths;
 import java.util.Map;
 import java.util.logging.Level;
-import java.util.logging.Logger;
 import java.util.stream.Collectors;
+import lombok.extern.java.Log;
 import persistent.bazel.client.WorkerKey;
 
 /**
@@ -27,9 +41,8 @@
  * workers, likely for debugging purposes, but need to revisit. (Can't remember fully since it was
  * so long ago!)
  */
+@Log
 public class PersistentExecutor {
-  private static final Logger logger = Logger.getLogger(PersistentExecutor.class.getName());
-
   private static final ProtoCoordinator coordinator =
       ProtoCoordinator.ofCommonsPool(getMaxWorkersPerKey());
 
@@ -54,7 +67,7 @@ private static int getMaxWorkersPerKey() {
     try {
       return Integer.parseInt(System.getenv("BUILDFARM_MAX_WORKERS_PER_KEY"));
     } catch (Exception ignored) {
-      logger.info(
+      log.info(
           "Could not get env var BUILDFARM_MAX_WORKERS_PER_KEY; defaulting to "
               + defaultMaxWorkersPerKey);
     }
@@ -79,17 +92,20 @@ public static Code runOnPersistentWorker(
       throws IOException {
     //// Pull out persistent worker start command from the overall action request
 
-    logger.log(Level.FINE, "executeCommandOnPersistentWorker[" + operationName + "]");
+    log.log(Level.FINE, "executeCommandOnPersistentWorker[" + operationName + "]");
 
     ImmutableList<String> initCmd = parseInitCmd(persistentWorkerInitCmd, argsList);
 
     String executionName = getExecutionName(argsList);
     if (executionName.isEmpty()) {
-      logger.log(Level.SEVERE, "Invalid Argument: " + argsList);
+      log.log(Level.SEVERE, "Invalid Argument: " + argsList);
       return Code.INVALID_ARGUMENT;
     }
 
     // TODO revisit why this was necessary in the first place
+    // (@wiwa) I believe the reason has to do with JavaBuilder workers not relying on env vars,
+    // as compared to rules_scala, only reading info from the argslist of each command.
+    // That would mean the Java worker keys should be invariant to the env vars we see.
     ImmutableMap<String, String> env;
     if (executionName.equals(JAVAC_EXEC_NAME)) {
       env = ImmutableMap.of();
@@ -153,7 +169,7 @@ public static Code runOnPersistentWorker(
     //// Run request
     //// Required file operations (in/out) are the responsibility of the coordinator
 
-    logger.log(Level.FINE, "Request with key: " + key);
+    log.log(Level.FINE, "Request with key: " + key);
     WorkResponse response;
     String stdErr = "";
     try {
@@ -171,8 +187,8 @@ public static Code runOnPersistentWorker(
               + request.getArgumentsList();
       String msg = "Exception while running request: " + e + debug + "\n\n";
 
-      logger.log(Level.SEVERE, msg);
-      e.printStackTrace();
+      log.log(Level.SEVERE, msg, e);
+
       response =
           WorkResponse.newBuilder()
               .setOutput(msg)
@@ -183,7 +199,7 @@ public static Code runOnPersistentWorker(
     //// Set results
 
     String responseOut = response.getOutput();
-    logger.log(Level.FINE, "WorkResponse.output: " + responseOut);
+    log.log(Level.FINE, "WorkResponse.output: " + responseOut);
 
     int exitCode = response.getExitCode();
     resultBuilder
@@ -195,7 +211,7 @@ public static Code runOnPersistentWorker(
       return Code.OK;
     }
 
-    logger.severe(
+    log.severe(
         "PersistentExecutor.runOnPersistentWorker Failed with code: "
             + exitCode
             + "\n"
@@ -226,7 +242,7 @@ private static ImmutableList<String> parseInitCmd(String cmdStr, ImmutableList<S
     // Parse init command into list of space-separated words, without the persistent worker flag
     ImmutableList.Builder<String> initCmdBuilder = ImmutableList.builder();
     for (String s : argsList) {
-      if (cmd.length() == 0) {
+      if (cmd.isEmpty()) {
         break;
       }
       cmd = cmd.substring(s.length()).trim();

src/main/java/build/buildfarm/worker/persistent/ProtoCoordinator.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import static persistent.bazel.client.PersistentWorker.TOOL_INPUT_SUBDIR;
@@ -14,7 +28,7 @@
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.logging.Level;
-import java.util.logging.Logger;
+import lombok.extern.java.Log;
 import persistent.bazel.client.CommonsWorkerPool;
 import persistent.bazel.client.PersistentWorker;
 import persistent.bazel.client.WorkCoordinator;
@@ -26,9 +40,8 @@
  * requirements, e.g. ensuring tool input files 3) post-response requirements, i.e. putting output
  * files in the right place
  */
+@Log
 public class ProtoCoordinator extends WorkCoordinator<RequestCtx, ResponseCtx, CommonsWorkerPool> {
-  private static final Logger logger = Logger.getLogger(ProtoCoordinator.class.getName());
-
   private static final String WORKER_INIT_LOG_SUFFIX = ".initargs.log";
 
   private static final ConcurrentHashMap<RequestCtx, PersistentWorker> pendingReqs =
@@ -119,7 +132,7 @@ private static String getUniqueSubdir(Path workRoot) {
   // copyToolInputsIntoWorkerToolRoot() should have been called before this.
   private static void copyToolsIntoWorkerExecRoot(WorkerKey key, Path workerExecRoot)
       throws IOException {
-    logger.log(Level.FINE, "loadToolsIntoWorkerRoot() into: " + workerExecRoot);
+    log.log(Level.FINE, "loadToolsIntoWorkerRoot() into: " + workerExecRoot);
 
     Path toolInputRoot = key.getExecRoot().resolve(TOOL_INPUT_SUBDIR);
     for (Path relPath : key.getWorkerFilesWithHashes().keySet()) {
@@ -188,9 +201,8 @@ private IOException logBadCleanup(RequestCtx request, IOException e) {
         .append("getOutputDirectoriesList:\n")
         .append(context.outputDirectories);
 
-    logger.severe(sb.toString());
+    log.log(Level.SEVERE, sb.toString(), e);
 
-    e.printStackTrace();
     return new IOException("Response was OK but failed on postWorkCleanup", e);
   }
 
@@ -216,11 +228,8 @@ void moveOutputsToOperationRoot(WorkFilesContext context, Path workerExecRoot)
     }
 
     for (String relOutput : context.outputFiles) {
-      System.out.println(relOutput);
       Path execOutputPath = workerExecRoot.resolve(relOutput);
-      System.out.println(execOutputPath);
       Path opOutputPath = opRoot.resolve(relOutput);
-      System.out.println(opOutputPath);
 
       FileAccessUtils.moveFile(execOutputPath, opOutputPath);
     }
@@ -258,11 +267,11 @@ public void run() {
 
   private void onTimeout(RequestCtx request, PersistentWorker worker) {
     if (worker != null) {
-      logger.severe("Persistent Worker timed out on request: " + request.request);
+      log.severe("Persistent Worker timed out on request: " + request.request);
       try {
         this.workerPool.invalidateObject(worker.getKey(), worker);
       } catch (Exception e) {
-        logger.severe(
+        log.severe(
             "Tried to invalidate worker for request:\n"
                 + request
                 + "\n\tbut got: "

src/main/java/build/buildfarm/worker/persistent/RequestCtx.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest;

src/main/java/build/buildfarm/worker/persistent/ResponseCtx.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse;

src/main/java/build/buildfarm/worker/persistent/WorkFilesContext.java

@@ -1,3 +1,17 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package build.buildfarm.worker.persistent;
 
 import build.bazel.remote.execution.v2.Command;