[GitHub] [beam] lostluck commented on a change in pull request #13484: [BEAM-11207] Return valid error on job failures

[GitBox <gi...@apache.org>]

lostluck commented on a change in pull request #13484:
URL: https://github.com/apache/beam/pull/13484#discussion_r536493356



##########
File path: sdks/go/pkg/beam/runners/universal/runnerlib/execute.go
##########
@@ -94,11 +94,16 @@ func Execute(ctx context.Context, p *pipepb.Pipeline, endpoint string, opt *JobO
 	}
 	err = WaitForCompletion(ctx, client, jobID)
 
-	res, err := newUniversalPipelineResult(ctx, jobID, client)
-	if err != nil {
-		return presult, err
+	res, presultErr := newUniversalPipelineResult(ctx, jobID, client)
+	if presultErr != nil {
+		if err != nil {
+			return presult, errors.Wrap(err, presultErr.Error())
+		} else {
+			return presult, presultErr
+		}
+	} else {
+		presult = res
 	}

Review comment:
       In Go, there's no reason to try to have a single return, or maintain an else. 
   In Go, prefer having the happiest path have the least indentation, and strive to return to that lowest level. Simply return, and avoid the extra indentation of an else.
   
   An if else block implies that either clause can be executed and those possibilities need to be kept in mind when reading later code. If the if returns (or continues, or breaks, as appropriate), there's no need for an else, as the code in the If never interacts with anything after it.
   
   ```suggestion
   	res, presultErr := newUniversalPipelineResult(ctx, jobID, client)
   	if presultErr != nil {
   		if err != nil {
   			return presult, errors.Wrap(err, presultErr.Error())
   		} 
   		return presult, presultErr
   	} 
   	return res, err
   ```
   This in combination with my other suggestion below to remove the other line.

##########
File path: sdks/go/pkg/beam/runners/universal/runnerlib/execute.go
##########
@@ -94,11 +94,16 @@ func Execute(ctx context.Context, p *pipepb.Pipeline, endpoint string, opt *JobO
 	}
 	err = WaitForCompletion(ctx, client, jobID)
 
-	res, err := newUniversalPipelineResult(ctx, jobID, client)
-	if err != nil {
-		return presult, err
+	res, presultErr := newUniversalPipelineResult(ctx, jobID, client)
+	if presultErr != nil {
+		if err != nil {
+			return presult, errors.Wrap(err, presultErr.Error())
+		} else {
+			return presult, presultErr
+		}
+	} else {
+		presult = res
 	}
-	presult = res
 
 	return presult, err
 }

Review comment:
       ```suggestion
   }
   ```
   
   cleanup of the old return that doesn't need to be preserved.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org