gemini-code-assist[bot] commented on code in PR #36068:
URL: https://github.com/apache/beam/pull/36068#discussion_r2330423441
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryHelpers.java:
##########
@@ -281,6 +283,21 @@ boolean pollJob() throws IOException {
boolean shouldRetry() {
return currentAttempt < maxRetries + 1;
}
+
+ void logBigQueryError(@Nullable Job job) {
+ if (job == null || !parseStatus(job).equals(Status.FAILED)) {
+ return;
+ }
+
+ List<ErrorProto> jobErrors = job.getStatus().getErrors();
+ String finalError = job.getStatus().getErrorResult().getMessage();
+ String causativeError =
+ jobErrors != null && !jobErrors.isEmpty()
+ ? String.format(" due to: %s", jobErrors.get(jobErrors.size() -
1).getMessage())
+ : "";
+
+ LOG.error(String.format("BigQuery Error : %s %s", finalError,
causativeError));
+ }
Review Comment:
The new `logBigQueryError` method has a few issues that could lead to
runtime errors and inefficient execution:
* **Potential `NullPointerException`**: `job.getStatus()` can return
`null`, which is not handled. The call to `parseStatus(job)` is unsafe as
`parseStatus` itself doesn't handle a null `JobStatus`. Even if it were safe,
subsequent calls to `job.getStatus()` within this method are also not null-safe.
* **Inefficiency**: `job.getStatus()` is called multiple times. It's
better to call it once and store the result in a local variable.
* **Incomplete error handling**: The logic assumes
`job.getStatus().getErrorResult()` is not null. If a job fails with errors in
the `errors` list but `errorResult` is null, this method would fail instead of
logging the available error information.
* **Log message formatting**: The use of `String.format("... %s %s", ...)`
can result in extraneous spaces in the log message.
Here is a revised implementation that addresses these points, making the
method safer, more efficient, and more robust.
```java
void logBigQueryError(@Nullable Job job) {
if (job == null) {
return;
}
final JobStatus status = job.getStatus();
if (status == null
|| (status.getErrorResult() == null
&& (status.getErrors() == null ||
status.getErrors().isEmpty()))) {
return;
}
String finalError;
if (status.getErrorResult() != null &&
status.getErrorResult().getMessage() != null) {
finalError = status.getErrorResult().getMessage();
} else {
finalError = "Job failed with an unknown error";
}
List<ErrorProto> jobErrors = status.getErrors();
String causativeErrorDetails = "";
if (jobErrors != null && !jobErrors.isEmpty()) {
causativeErrorDetails = " due to: " + jobErrors.get(jobErrors.size()
- 1).getMessage();
}
LOG.error(String.format("BigQuery Error : %s%s", finalError,
causativeErrorDetails));
}
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
