Skip to content
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.

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

feat: pass retry from Job.result() to Job.done() #41

Merged
merged 16 commits into from Nov 3, 2020

Conversation

@IlyaFaer
Copy link
Member

@IlyaFaer IlyaFaer commented Feb 21, 2020

Closes #24

@IlyaFaer IlyaFaer changed the title feat(bigquery): pass retry from Job.result() to Job.done(). feat(bigquery): pass retry from Job.result() to Job.done() Feb 21, 2020
@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Feb 21, 2020

This PR is strictly connected to googleapis/python-api-core#9, and should not be merged before №9

Loading

@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Feb 21, 2020

Here is how it's gonna be passed into done()

image

Loading

@tseaver tseaver changed the title feat(bigquery): pass retry from Job.result() to Job.done() feat: pass retry from Job.result() to Job.done() Jul 30, 2020
@busunkim96 busunkim96 closed this Jul 31, 2020
@busunkim96 busunkim96 reopened this Jul 31, 2020
@tswast
Copy link
Contributor

@tswast tswast commented Oct 20, 2020

This will need to be updated to set the minimum api-core version in setup.py to 1.23.0 after googleapis/python-api-core#96 is released.

Loading

@tswast
Copy link
Contributor

@tswast tswast commented Oct 21, 2020

1.23.0 has been released. Work can continue on this PR

Loading

@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Oct 22, 2020

Only one error is appearing:

self = <google.cloud.bigquery.job._AsyncJob object at 0x7f1fc2f0eb70>

    def to_api_repr(self):
        """Generate a resource for the job."""
>       raise NotImplementedError("Abstract")
E       NotImplementedError: Abstract

Doesn't seem to be related.

Loading

@IlyaFaer IlyaFaer requested a review from tswast Oct 22, 2020
@IlyaFaer IlyaFaer marked this pull request as ready for review Oct 22, 2020
@IlyaFaer IlyaFaer requested a review from as a code owner Oct 22, 2020
Copy link
Contributor

@tswast tswast left a comment

Missing update to minimum versions.

Loading

google/cloud/bigquery/job.py Show resolved Hide resolved
Loading
@tswast
Copy link
Contributor

@tswast tswast commented Oct 23, 2020

Filed #339 to look into the test failure. I've assigned to myself, as it might be related to some refactoring I recently completed.

Loading

@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Oct 27, 2020

I think, a lot of tests will fail. And in many tests we'll have to add one more arg. That's why I've added the kwargs pattern.

Loading

@tswast
Copy link
Contributor

@tswast tswast commented Oct 27, 2020

I think, a lot of tests will fail. And in many tests we'll have to add one more arg. That's why I've added the kwargs pattern.

Okay, that makes sense. So long as we have some tests that have a custom retry, I'm okay with only populating it when it's something other than the default.

Loading

Copy link
Contributor

@tswast tswast left a comment

I'm not seeing any tests that verify the retry behavior is working as expected.

I'd like to see the test update I suggested here: https://github.com/googleapis/python-bigquery/pull/340/files#r511017196

The QueryJob class overrides result(), so we'll need a test that does the same but for QueryJob specifically.

Loading

@google-cla
Copy link

@google-cla google-cla bot commented Nov 2, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Loading

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 2, 2020
@HemangChothani
Copy link
Contributor

@HemangChothani HemangChothani commented Nov 2, 2020

@googlebot I consent

Loading

@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Nov 2, 2020

@tswast, sorry, I'm little bit besieged by tasks, not able to continue with this right now. @HemangChothani kindly agreed to take a look at this PR.

Loading

tests/unit/test_job.py Show resolved Hide resolved
Loading
tswast
tswast approved these changes Nov 3, 2020
@tswast tswast merged commit 284e17a into googleapis:master Nov 3, 2020
10 checks passed
Loading
@IlyaFaer IlyaFaer deleted the retry_from_result_to_done branch Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants