Skip to content
This repository was archived by the owner on Feb 7, 2026. It is now read-only.

fix(job): check for errorResult when polling jobs#387

Merged
callmehiphop merged 3 commits intogoogleapis:masterfrom
callmehiphop:dg--329
Mar 20, 2019
Merged

fix(job): check for errorResult when polling jobs#387
callmehiphop merged 3 commits intogoogleapis:masterfrom
callmehiphop:dg--329

Conversation

@callmehiphop
Copy link
Copy Markdown
Contributor

@callmehiphop callmehiphop commented Mar 18, 2019 •
edited
Loading

Fixes #329

Would like some confirmation from a BQ team member on this one.

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 18, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 18, 2019 •
edited
Loading

Codecov Report

Merging #387 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #387   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files           4        4           
  Lines         544      544           
  Branches       75       75           
=======================================
  Hits          541      541           
  Misses          2        2           
  Partials        1        1
Impacted Files Coverage Δ
src/job.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5721fd1...3284913. Read the comment docs.

@stephenplusplus
Copy link
Copy Markdown
Contributor

stephenplusplus commented Mar 18, 2019

It does seem correct to use errorResults, considering these definitions:

status.errorResults

Final error result of the job. If present, indicates that the job has completed and was unsuccessful. For more information, see troubleshooting errors.

status.errors[]

The first errors encountered during the running of the job. The final message includes the number of errors that caused the process to stop. Errors here do not necessarily mean that the job has completed or was unsuccessful.

Although, I wonder what type of error could happen in status.errors[] that wouldn't be means for the job to be unsuccessful? That seems like it would be more of a warning as opposed to an error.

@callmehiphop
Copy link
Copy Markdown
Contributor Author

callmehiphop commented Mar 18, 2019

@stephenplusplus so while investigating #329 I learned that you can set a maxBadRecords option while uploading a CSV file. Essentially you can have a certain number of erroneous rows and for each error an object will appear in the errors array. If you come in under the maxBadRecords value then errorResult will not be present and the job is considered a success.

bcoe
bcoe approved these changes Mar 18, 2019
View reviewed changes
Copy link
Copy Markdown

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

If I'm understanding this correctly, in the past if we received an error and a response we preferred the response? now error takes precedence?

This looks good to me; I'd probably lean towards calling it a breaking change? (wouldn't be shocked if people relied on the old behavior).

bcoe
bcoe reviewed Mar 18, 2019
View reviewed changes
Comment thread test/job.ts
beforeEach(() => {
job.getMetadata = (callback: Function) => {
callback(null, apiResponse, apiResponse);
callback(null, apiResponse);
Copy link
Copy Markdown

@bcoe bcoe Mar 18, 2019

Choose a reason for hiding this comment

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

nit: it would be cool if we could get a test case around this change in behavior.

Copy link
Copy Markdown
Contributor Author

@callmehiphop callmehiphop Mar 18, 2019

Choose a reason for hiding this comment

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

I actually thought this was a behavior change that wasn't pulled over to BigQuery correctly, but now that you said this I think we might have stumbled on to a bug in nodejs-common

Copy link
Copy Markdown
Contributor Author

@callmehiphop callmehiphop Mar 18, 2019

Choose a reason for hiding this comment

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

Oh, nevermind, my initial guess was right. There was a breaking change in common 0.28.0 and the fix apparently never found its way here.

tswast
tswast approved these changes Mar 19, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Change to check for errorResult instead of any errors LGTM.

@callmehiphop
Copy link
Copy Markdown
Contributor Author

callmehiphop commented Mar 20, 2019

@bcoe somehow I missed your comment, but the response can contain 2 different error fields. errorResult and errors. As it is today we check for errors and assume the job failed. I just learned that errors can in some cases serve more as a warning and that errorResult is more correct in determining if the job failed. When both errors and errorResult are present, the first item in errors (array) is actually the same value as errorResult, so users will still get the same exact error.

@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 20, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 20, 2019
@callmehiphop
Copy link
Copy Markdown
Contributor Author

callmehiphop commented Mar 20, 2019 •
edited
Loading

@tswast so this error started popping up today

No matching signature for operator < for argument types: STRUCT<seconds INT64, nanos INT64>, TIMESTAMP. Supported signatures: ANY < ANY

Is there something going on with the backend, or should we be preparing a fix for this??

@tswast
Copy link
Copy Markdown
Contributor

tswast commented Mar 20, 2019

This looks like a problem in the back end handling of query parameters, unrelated to this PR. Thanks for the heads up. Investigating.

@callmehiphop callmehiphop merged commit 1bf9e89 into googleapis:master Mar 20, 2019
@release-please release-please Bot mentioned this pull request May 18, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@bcoe bcoe bcoe approved these changes

@tswast tswast tswast approved these changes

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@callmehiphop @stephenplusplus @tswast @bcoe @JustinBeckwith @googlebot @kokoro-team