Skip to content

fix: Job.isDone() uses Job.Status.State if available #4039

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
lqiu96 merged 1 commit into from
Dec 19, 2025
Merged

fix: Job.isDone() uses Job.Status.State if available #4039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Job.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ public boolean exists() {
*/
public boolean isDone() {
checkNotDryRun("isDone");
if (hasDoneState()) {
return true;
}
Span isDone = null;
if (options.isOpenTelemetryTracingEnabled() && options.getOpenTelemetryTracer() != null) {
isDone =
Expand All @@ -220,14 +223,18 @@ public boolean isDone() {
}
try (Scope isDoneScope = isDone != null ? isDone.makeCurrent() : null) {
Job job = bigquery.getJob(getJobId(), JobOption.fields(BigQuery.JobField.STATUS));
return job == null || JobStatus.State.DONE.equals(job.getStatus().getState());
return job == null || job.hasDoneState();
Copy link
Member

@lqiu96 lqiu96 Dec 18, 2025

Choose a reason for hiding this comment

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

@PhongChuong @jinseopkim0

Do either of you recall the logic behind returning true if the job is not found (job == null)?

Copy link
Member

@lqiu96 lqiu96 Dec 18, 2025

Choose a reason for hiding this comment

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

Logic is the same and code changes look fine. I'm a bit confused why we return true if the job isn't found (I'm fine with looking into this behavior in a separate issue if we're all not sure about this)

Copy link
Contributor

@PhongChuong PhongChuong Dec 18, 2025

Choose a reason for hiding this comment

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

I don't know the reason why isDone historically returns true when the job isn't found but as it is part of the function defined behavior, I think it's best to leave it as it is currently.

Copy link
Member

@lqiu96 lqiu96 Dec 18, 2025

Choose a reason for hiding this comment

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

Thanks, I agree. Let's keep this behavior the same.

I'll create a separate issue to investigate this behavior and see if there is any additional context behind this decision.

} finally {
if (isDone != null) {
isDone.end();
}
}
}

private boolean hasDoneState() {
return getStatus() != null && JobStatus.State.DONE.equals(getStatus().getState());
}

/** See {@link #waitFor(BigQueryRetryConfig, RetryOption...)} */
public Job waitFor(RetryOption... waitOptions) throws InterruptedException {
return waitForInternal(DEFAULT_RETRY_CONFIG, waitOptions);
Expand Down
8 changes: 4 additions & 4 deletions google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/JobTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,9 @@ public void testExists_False() {

@Test
public void testIsDone_True() {
BigQuery.JobOption[] expectedOptions = {BigQuery.JobOption.fields(BigQuery.JobField.STATUS)};
Job job = expectedJob.toBuilder().setStatus(new JobStatus(JobStatus.State.DONE)).build();
when(bigquery.getJob(JOB_INFO.getJobId(), expectedOptions)).thenReturn(job);
assertTrue(job.isDone());
verify(bigquery).getJob(JOB_INFO.getJobId(), expectedOptions);
verify(bigquery, times(0)).getJob(eq(JOB_INFO.getJobId()), any());
}

@Test
Expand All @@ -176,8 +174,10 @@ public void testIsDone_False() {
@Test
public void testIsDone_NotExists() {
BigQuery.JobOption[] expectedOptions = {BigQuery.JobOption.fields(BigQuery.JobField.STATUS)};
Job jobWithRunningState =
expectedJob.toBuilder().setStatus(new JobStatus(JobStatus.State.RUNNING)).build();
when(bigquery.getJob(JOB_INFO.getJobId(), expectedOptions)).thenReturn(null);
assertTrue(job.isDone());
assertTrue(jobWithRunningState.isDone());
verify(bigquery).getJob(JOB_INFO.getJobId(), expectedOptions);
}

Expand Down