Skip to content

Logs the BigQuery error that caused the load job failure (resolves #1… #36068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
damccorm merged 2 commits into from
Sep 8, 2025
Merged

Logs the BigQuery error that caused the load job failure (resolves #1… #36068

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b64287c
Logs the BigQuery error that caused the load job failure (resolves #1…
mburuian6 Sep 5, 2025
43bd1d0
Checks that the BigQuery root cause message to be logged is not empty…
mburuian6 Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions ...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.api.client.util.Sleeper;
import com.google.api.services.bigquery.model.Clustering;
import com.google.api.services.bigquery.model.Dataset;
import com.google.api.services.bigquery.model.ErrorProto;
import com.google.api.services.bigquery.model.Job;
import com.google.api.services.bigquery.model.JobReference;
import com.google.api.services.bigquery.model.JobStatus;
Expand Down Expand Up @@ -205,6 +206,7 @@ static class PendingJob implements Serializable {
void runJob() throws IOException {
++currentAttempt;
if (!shouldRetry()) {
logBigQueryError(lastJobAttempted);
throw new RuntimeException(
String.format(
"Failed to create job with prefix %s, "
Expand Down Expand Up @@ -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));
}
Comment on lines +287 to +300
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

    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));
    }

Copy link
Contributor

@damccorm damccorm Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null issues called out here aren't real, these methods all return non-null results.

The efficiency piece doesn't matter in the failure condition, and the formatting should be fine

}

static class RetryJobId implements Serializable {
Expand Down
Loading