PR feedback

enterprise/server/hostedrunner/hostedrunner.go

@@ -222,7 +222,7 @@ func (r *runnerService) createAction(ctx context.Context, req *rnpb.RunRequest,
 				{Name: platform.EstimatedComputeUnitsPropertyName, Value: "3"},
 				{Name: platform.EstimatedFreeDiskPropertyName, Value: "20000000000"}, // 20GB
 				{Name: platform.DockerUserPropertyName, Value: user},
-				{Name: platform.ShouldRetryPropertyName, Value: fmt.Sprintf("%v", retry)},
+				{Name: platform.RetryPropertyName, Value: fmt.Sprintf("%v", retry)},
 			},
 		},
 	}

enterprise/server/remote_execution/executor/executor.go

@@ -150,12 +150,12 @@ func isClientBazel(task *repb.ExecutionTask) bool {
 }
 
 func shouldRetry(task *repb.ExecutionTask, taskError error) bool {
-	if !platform.RetriesEnabled(task) {
+	if !platform.Retryable(task) {
 		return false
 	}
 
 	// If the task is invalid / misconfigured, more attempts won't help.
-	if status.IsTaskMisconfigured(taskError) {
+	if !status.Retryable(taskError) {
 		return false
 	}
 	// If the task timed out, respect the timeout and don't keep retrying.

enterprise/server/remote_execution/platform/platform.go

@@ -96,7 +96,7 @@ const (
 	IncludeSecretsPropertyName           = "include-secrets"
 	DefaultTimeoutPropertyName           = "default-timeout"
 	TerminationGracePeriodPropertyName   = "termination-grace-period"
-	ShouldRetryPropertyName              = "should-retry"
+	RetryPropertyName                    = "retry"
 
 	OperatingSystemPropertyName = "OSFamily"
 	LinuxOperatingSystemName    = "linux"
@@ -244,13 +244,13 @@ type Properties struct {
 	// applied as overrides to the action.
 	EnvOverrides []string
 
-	// ShouldRetry determines whether the scheduler should automatically retry
+	// Retry determines whether the scheduler should automatically retry
 	// transient errors.
 	// Should be set to false for non-idempotent commands, and clients should
 	// handle more fine-grained retry behavior.
 	// This property is ignored for bazel executions, because the bazel client
 	// handles retries itself.
-	ShouldRetry bool
+	Retry bool
 }
 
 // ContainerType indicates the type of containerization required by an executor.
@@ -379,7 +379,7 @@ func ParseProperties(task *repb.ExecutionTask) (*Properties, error) {
 		DisablePredictedTaskSize:  boolProp(m, disablePredictedTaskSizePropertyName, false),
 		ExtraArgs:                 stringListProp(m, extraArgsPropertyName),
 		EnvOverrides:              envOverrides,
-		ShouldRetry:               boolProp(m, ShouldRetryPropertyName, true),
+		Retry:                     boolProp(m, RetryPropertyName, true),
 	}, nil
 }
 
@@ -779,7 +779,7 @@ func IsCICommand(cmd *repb.Command, platform *repb.Platform) bool {
 	return false
 }
 
-func RetriesEnabled(task *repb.ExecutionTask) bool {
-	v := FindEffectiveValue(task, ShouldRetryPropertyName)
+func Retryable(task *repb.ExecutionTask) bool {
+	v := FindEffectiveValue(task, RetryPropertyName)
 	return v == "true" || v == ""
 }

enterprise/server/scheduling/scheduler_server/scheduler_server.go

@@ -2023,15 +2023,15 @@ func (s *SchedulerServer) reEnqueueTask(ctx context.Context, taskID, leaseID, re
 	if err := proto.Unmarshal(scheduledTask.serializedTask, task); err != nil {
 		return status.InternalErrorf("failed to unmarshal ExecutionTask: %s", err)
 	}
-	if !platform.RetriesEnabled(task) {
+	if !platform.Retryable(task) {
 		if _, err := s.deleteTask(ctx, taskID); err != nil {
 			return err
 		}
 		msg := fmt.Sprintf("Task %q does not have retries enabled. Not re-enqueuing. Last failure: %s", taskID, reason)
+		log.Infof(msg)
 		if err := s.env.GetRemoteExecutionService().MarkExecutionFailed(ctx, taskID, status.InternalError(msg)); err != nil {
 			log.CtxWarningf(ctx, "Could not mark execution failed for task %q: %s", taskID, err)
 		}
-		log.Infof(msg)
 		return nil
 	}
 

enterprise/server/scheduling/scheduler_server/scheduler_server_test.go

@@ -555,7 +555,7 @@ func TestExecutorReEnqueue_RetriesDisabled(t *testing.T) {
 	fe := newFakeExecutor(ctx, t, env.GetSchedulerClient())
 	fe.Register()
 
-	taskID := scheduleTask(ctx, t, env, map[string]string{platform.ShouldRetryPropertyName: "false"})
+	taskID := scheduleTask(ctx, t, env, map[string]string{platform.RetryPropertyName: "false"})
 	fe.WaitForTask(taskID)
 	lease := fe.Claim(taskID)
 	fe.ResetTasks()

enterprise/server/workflow/service/service.go

@@ -1218,7 +1218,7 @@ func (ws *workflowService) createActionForWorkflow(ctx context.Context, wf *tabl
 				{Name: platform.EstimatedFreeDiskPropertyName, Value: estimatedDisk},
 				{Name: platform.EstimatedMemoryPropertyName, Value: workflowAction.ResourceRequests.GetEstimatedMemory()},
 				{Name: platform.EstimatedCPUPropertyName, Value: workflowAction.ResourceRequests.GetEstimatedCPU()},
-				{Name: platform.ShouldRetryPropertyName, Value: fmt.Sprintf("%v", retry)},
+				{Name: platform.RetryPropertyName, Value: fmt.Sprintf("%v", retry)},
 			},
 		},
 	}

server/util/status/status.go

@@ -264,10 +264,11 @@ func MetricsLabel(err error) string {
 	return status.Code(err).String()
 }
 
-// IsTaskMisconfigured returns whether the error is a configuration error
+// Retryable returns false if the error is a configuration error
 // that will persist, even despite retries.
-func IsTaskMisconfigured(err error) bool {
-	return IsInvalidArgumentError(err) ||
+func Retryable(err error) bool {
+	taskMisconfigured := IsInvalidArgumentError(err) ||
 		IsFailedPreconditionError(err) ||
 		IsUnauthenticatedError(err)
+	return !taskMisconfigured
 }