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

BigQuery: add 'retry' argument to '_AsyncJob.result'. #6302

Merged
merged 1 commit into from
Dec 1, 2018
Merged

BigQuery: add 'retry' argument to '_AsyncJob.result'. #6302

merged 1 commit into from
Dec 1, 2018

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 25, 2018

Pass it through to the _begin call. Note that we need to modify
the api_core...PollingFuture class before we can safely pass the
retry through to its result.

@tseaver tseaver added the api: bigquery Issues related to the BigQuery API. label Oct 25, 2018
@tseaver tseaver requested a review from tswast October 25, 2018 16:12
@tseaver tseaver requested a review from crwilcox as a code owner October 25, 2018 16:12
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 25, 2018
Copy link
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.

LGTM, though I'll want to hear what Chris or Thea thinks since this is a divergence from the standard "futures" interface. Maybe we should store a private Retry object on the Job class instead? We already override done() for some of the job types.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 25, 2018

@tswast The PollingFuture base class already stores a retry object. If we assigned to it, we would need to ensure that its predicate included the test for _OperationNotComplete (see the DEFAULT_RETRY it uses).

@tseaver
Copy link
Contributor Author

tseaver commented Oct 25, 2018

I had actually planned to update the PollingFuture.result method to take the optional retry argument as well, but wanted to think about how to deal with the _OperationNotComplete bit first.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 25, 2018

@tswast Please see #6305 for the changes to retry the polling requests. I haven't piped through the retry argument from _AsyncJob.result(), because of issues related to _OperationNotComplete.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 29, 2018
@tseaver
Copy link
Contributor Author

tseaver commented Oct 29, 2018

If we close this PR without merging (i.e., we decide not to add a retry argument to _AsyncJob.result), then we should change _AsyncJob.result to pass through self._retry to the _begin call. That change won't really be useful until #6305 lands, however.

@crwilcox
Copy link
Contributor

@tseaver is this needed at all if we do #6305?

@tseaver
Copy link
Contributor Author

tseaver commented Oct 30, 2018

@crwilcox

is this needed at all if we do #6305?

Depends The _AsyncJob.result method itself can invoke _begin, which takes an optional retry argument. It makes sense to me that we plumb that through so that callers of result can override the default.

If we decide not to expose it, then I think we still need to pass the self._retry attribute through to begin, because the application might've configured it to override e.g., the timeout from the defaults, via something like:

from google.cloud.bigquery import job

...
copy_job = job.CopyJob(
    job_ref, sources, destination, client=client, job_config=job_config)
copy_job._retry = copy_job._retry.with_deadline(600)
result = copy_job.result()

@tseaver tseaver requested a review from theacodes October 31, 2018 14:16
Pass it through to the '_begin' call.  Note that we need to modify
the 'api_core...PollingFuture' class before we can safely pass the
'retry' through to its 'result'.
@tseaver
Copy link
Contributor Author

tseaver commented Nov 19, 2018

Rebased to work around borked CI runs for irrelevant APIs.

@tseaver tseaver merged commit be78a96 into googleapis:master Dec 1, 2018
@tseaver tseaver deleted the bigquery-job_result_retry_part_one branch December 1, 2018 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants