Make bes_best_effort a no-op

The current semantics of the flag is to allow BES upload to continue
past the nominal end of the build invocation, and possibly overlapping
a subsequent build invocation. This conflicts with file uploads, which
must read the file before it is removed or modified by the subsequent
build invocation. On Linux, we could just open a file handle, but this
isn't possible on Windows.

We decided to make the flag a no-op for now. Note that the default is
already set to false. We may resurrect this option in the future if
there's a strong use case for it, possibly with slightly different
semantics.

PiperOrigin-RevId: 199620392

3 files changed, 39 insertions, 57 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java
index a4e295751c..513879243a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java
@@ -176,14 +176,10 @@ public abstract class BuildEventServiceModule<T extends BuildEventServiceOptions
                 commandLineReporter,
                 startupOptionsProvider);
       } catch (Exception e) {
-        if (besOptions.besBestEffort) {
-          commandLineReporter.handle(Event.warn(format(UPLOAD_FAILED_MESSAGE, e.getMessage())));
-        } else {
-          commandLineReporter.handle(Event.error(format(UPLOAD_FAILED_MESSAGE, e.getMessage())));
-          moduleEnvironment.exit(new AbruptExitException(
-              "Failed while creating BuildEventTransport", ExitCode.PUBLISH_ERROR));
-          return null;
-        }
+        commandLineReporter.handle(Event.error(format(UPLOAD_FAILED_MESSAGE, e.getMessage())));
+        moduleEnvironment.exit(new AbruptExitException(
+            "Failed while creating BuildEventTransport", ExitCode.PUBLISH_ERROR));
+        return null;
       }
 
       ImmutableSet<BuildEventTransport> bepTransports =
@@ -247,7 +243,6 @@ public abstract class BuildEventServiceModule<T extends BuildEventServiceOptions
           new BuildEventServiceTransport(
               createBesClient(besOptions, authTlsOptions),
               besOptions.besTimeout,
-              besOptions.besBestEffort,
               besOptions.besLifecycleEvents,
               buildRequestId,
               invocationId,
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceOptions.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceOptions.java
index df9436d86f..ce99eee395 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceOptions.java
@@ -51,14 +51,16 @@ public class BuildEventServiceOptions extends OptionsBase {
   public Duration besTimeout;
 
   @Option(
-    name = "bes_best_effort",
-    defaultValue = "false",
-    documentationCategory = OptionDocumentationCategory.LOGGING,
-    effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
-    help =
-        "Specifies whether a failure to upload the BES protocol should also result in a build "
-            + "failure. If 'false', bazel exits with ExitCode.PUBLISH_ERROR. (defaults to 'false')."
-  )
+      name = "bes_best_effort",
+      defaultValue = "false",
+      deprecationWarning =
+          "BES best effort upload has been removed. The flag has no more "
+              + "functionality attached to it and will be removed in a future release.",
+      documentationCategory = OptionDocumentationCategory.LOGGING,
+      effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+      help =
+          "BES best effort upload has been removed. The flag has no more "
+              + "functionality attached to it and will be removed in a future release.")
   public boolean besBestEffort;
 
   @Option(
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java
index ee5c654d95..42c363b49f 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java
@@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.util.concurrent.MoreExecutors.listeningDecorator;
 import static com.google.devtools.build.lib.events.EventKind.ERROR;
 import static com.google.devtools.build.lib.events.EventKind.INFO;
-import static com.google.devtools.build.lib.events.EventKind.WARNING;
 import static com.google.devtools.build.v1.BuildEvent.EventCase.COMPONENT_STREAM_FINISHED;
 import static com.google.devtools.build.v1.BuildStatus.Result.COMMAND_FAILED;
 import static com.google.devtools.build.v1.BuildStatus.Result.COMMAND_SUCCEEDED;
@@ -90,7 +89,6 @@ public class BuildEventServiceTransport implements BuildEventTransport {
   private final ListeningExecutorService uploaderExecutorService;
   private final Duration uploadTimeout;
   private final boolean publishLifecycleEvents;
-  private final boolean bestEffortUpload;
   private final BuildEventServiceClient besClient;
   private final BuildEventServiceProtoUtil besProtoUtil;
   private final ModuleEnvironment moduleEnvironment;
@@ -125,7 +123,6 @@ public class BuildEventServiceTransport implements BuildEventTransport {
   public BuildEventServiceTransport(
       BuildEventServiceClient besClient,
       Duration uploadTimeout,
-      boolean bestEffortUpload,
       boolean publishLifecycleEvents,
       String buildRequestId,
       String invocationId,
@@ -137,16 +134,27 @@ public class BuildEventServiceTransport implements BuildEventTransport {
       EventHandler commandLineReporter,
       @Nullable String projectId,
       Set<String> keywords) {
-    this(besClient, uploadTimeout, bestEffortUpload, publishLifecycleEvents, buildRequestId,
-        invocationId, command, moduleEnvironment, clock, protocolOptions, pathConverter,
-        commandLineReporter, projectId, keywords, new JavaSleeper());
+    this(
+        besClient,
+        uploadTimeout,
+        publishLifecycleEvents,
+        buildRequestId,
+        invocationId,
+        command,
+        moduleEnvironment,
+        clock,
+        protocolOptions,
+        pathConverter,
+        commandLineReporter,
+        projectId,
+        keywords,
+        new JavaSleeper());
   }
 
   @VisibleForTesting
   public BuildEventServiceTransport(
       BuildEventServiceClient besClient,
       Duration uploadTimeout,
-      boolean bestEffortUpload,
       boolean publishLifecycleEvents,
       String buildRequestId,
       String invocationId,
@@ -177,7 +185,6 @@ public class BuildEventServiceTransport implements BuildEventTransport {
     this.pathConverter = pathConverter;
     this.invocationResult = UNKNOWN_STATUS;
     this.uploadTimeout = uploadTimeout;
-    this.bestEffortUpload = bestEffortUpload;
     this.sleeper = sleeper;
   }
 
@@ -229,33 +236,17 @@ public class BuildEventServiceTransport implements BuildEventTransport {
               return;
             }
 
-            if (bestEffortUpload) {
-              // TODO(buchgr): The code structure currently doesn't allow to enforce a timeout for
-              // best effort upload.
-              if (!uploadComplete.isDone()) {
-                report(INFO, "Asynchronous Build Event Protocol upload.");
+            report(INFO, "Waiting for Build Event Protocol upload to finish.");
+            try {
+              if (Duration.ZERO.equals(uploadTimeout)) {
+                uploadComplete.get();
               } else {
-                Throwable uploadError = fromFuture(uploadComplete);
-
-                if (uploadError != null) {
-                  report(WARNING, UPLOAD_FAILED_MESSAGE, uploadError.getMessage());
-                } else {
-                  report(INFO, UPLOAD_SUCCEEDED_MESSAGE);
-                }
-              }
-            } else {
-              report(INFO, "Waiting for Build Event Protocol upload to finish.");
-              try {
-                if (Duration.ZERO.equals(uploadTimeout)) {
-                  uploadComplete.get();
-                } else {
-                  uploadComplete.get(uploadTimeout.toMillis(), MILLISECONDS);
-                }
-                report(INFO, UPLOAD_SUCCEEDED_MESSAGE);
-              } catch (Exception e) {
-                uploadComplete.cancel(true);
-                reportErrorAndFailBuild(e);
+                uploadComplete.get(uploadTimeout.toMillis(), MILLISECONDS);
               }
+              report(INFO, UPLOAD_SUCCEEDED_MESSAGE);
+            } catch (Exception e) {
+              uploadComplete.cancel(true);
+              reportErrorAndFailBuild(e);
             }
           } finally {
             shutdownFuture.set(null);
@@ -347,8 +338,6 @@ public class BuildEventServiceTransport implements BuildEventTransport {
   }
 
   private void reportErrorAndFailBuild(Throwable t) {
-    checkState(!bestEffortUpload);
-
     String message = errorMessageFromException(t);
 
     report(ERROR, message);
@@ -364,11 +353,7 @@ public class BuildEventServiceTransport implements BuildEventTransport {
     Throwable uploadError = fromFuture(uploadComplete);
     if (uploadError != null) {
       errorsReported = true;
-      if (bestEffortUpload) {
-        report(WARNING, UPLOAD_FAILED_MESSAGE, uploadError.getMessage());
-      } else {
-        reportErrorAndFailBuild(uploadError);
-      }
+      reportErrorAndFailBuild(uploadError);
     }
   }