bigquery: retry idempotent RPCs

[jba]

Add retry logic to every RPC for which it makes sense.

Following the BigQuery team, we ignore the error code and
use the "reason" field of the error to determine whether
to retry.

[jba]

This is a first cut at adding retry to the BigQuery client. My thinking is that most retry parameters are not interesting to users and should not be exposed, but the timeout (a.k.a. deadline) is important, and should be an optional parameter.

Another choice would be to allow an optional google.gax.CallOptions, as the generated code does.

[theacodes]

@jba this is a good start, but needs to be slightly different (and I apologize for not documenting what the retry pattern should look like).

1. All methods that retry should take a full retry argument that should be a retry instance.
2. A sane default retry should be provided.
3. Retry can be set to False or None to disable retrying.
4. The connection's api call should handle raising the appropriate google.api.core.exceptions.GoogleAPICallError subclass. This way, you don't need custom should_retry and users will get the appropriate exception if retrying fails.

So get_dataset should look something like this:

def get_dataset(self, dataset_ref, retry=DEFAULT_RETRY):
    api_call = functools.partial(
        self._connection.api_request,
        method='GET',
        path=dataset_ref.path)

    if retry:
        api_call = retry(api_call)

    api_response = api_call()

    return Dataset.from_api_repr(api_response)

and self._connection.api_request should handle raising the appropriate exception on error.

(FYI: functools.partial is preferred to lambda because retry et al knows how to properly print this when errors occur)

[jba]

Thanks for the explanation. The one thing I'm not clear about is the exception subclass. BigQuery retry ignores the error code and is based solely on the reason string in the error. Should I define a special BigQueryTransientError subclass to capture that, which wraps the original error? If so, I'll still need a custom predicate, won't I? And won't that be worse for users when retry fails, because they'll have to call RetryError.cause() and then call cause() on that again?

[theacodes]

BigQuery retry ignores the error code and is based solely on the reason string in the error.

Yikes. Do the error codes at least match up with idempotent http statuses?

Should I define a special BigQueryTransientError subclass to capture that, which wraps the original error?

Yeah, that could be reasonable, something like:

class BigQueryTransientError(exceptions.BackendError):
     ...

If so, I'll still need a custom predicate, won't I?

Possibly, if the default predicate doesn't work (e.g. if not all BackendErrors are retryable, just the subset that ends up being BigQueryTransientErrors). If that's the case, you can make it global:

google.cloud.bigquery.default_retry_predicate

That way custom retries are a bit easier:

myretry = retry.Retry(bigquery.default_retry_predicate, deadline=60)

You can also make the default retry object itself a global or class constant, so users can do;

myretry = bigquery_client.DEFAULT_RETRY.with_deadline(60)

And won't that be worse for users when retry fails, because they'll have to call RetryError.cause() and then call cause() on that again?

If you sublcass one of the existing exception classes they should only need to call cause() once, unless I'm misunderstanding something.

[jba]

Yikes. Do the error codes at least match up with idempotent http statuses?

The error codes are irrelevant. Only the reason field matters. So I guess the answer is no.

If so, I'll still need a custom predicate, won't I?

Possibly, if the default predicate doesn't work (e.g. if not all BackendErrors are retryable, just the subset that ends up being BigQueryTransientErrors).

The default predicate won't work, for the reason you said.

You can also make the default retry object itself a global or class constant, so users can do;

myretry = bigquery_client.DEFAULT_RETRY.with_deadline(60)

Done.

[jba]

I don't understand the error I'm getting. It happens only under 2.7. Am I holding functools wrong?

Traceback (most recent call last):
  File "/var/code/gcp/bigquery/tests/unit/test_client.py", line 297, in test_get_dataset
    dataset = client.get_dataset(dataset_ref)
  File "/var/code/gcp/bigquery/google/cloud/bigquery/client.py", line 288, in get_dataset
    api_call = retry(api_call)
  File "/var/code/gcp/.nox/unit-2-7/lib/python2.7/site-packages/google/api/core/retry.py", line 247, in __call__
    @six.wraps(func)
  File "/var/code/gcp/.nox/unit-2-7/lib/python2.7/site-packages/six.py", line 811, in wrapper
    f = functools.wraps(wrapped, assigned, updated)(f)
  File "/usr/local/lib/python2.7/functools.py", line 33, in update_wrapper
    setattr(wrapper, attr, getattr(wrapped, attr))
AttributeError: 'functools.partial' object has no attribute '__module__'

[jba]

PTAL. In the latest push I added retry to nearly all methods that can support it.

[lukesneeringer]

@jba

I don't understand the error I'm getting. It happens only under 2.7. Am I holding functools wrong?

Traceback (most recent call last):
  File "/var/code/gcp/bigquery/tests/unit/test_client.py", line 297, in test_get_dataset
    dataset = client.get_dataset(dataset_ref)
  File "/var/code/gcp/bigquery/google/cloud/bigquery/client.py", line 288, in get_dataset
    api_call = retry(api_call)
  File "/var/code/gcp/.nox/unit-2-7/lib/python2.7/site-packages/google/api/core/retry.py", line 247, in __call__
    @six.wraps(func)
  File "/var/code/gcp/.nox/unit-2-7/lib/python2.7/site-packages/six.py", line 811, in wrapper
    f = functools.wraps(wrapped, assigned, updated)(f)
  File "/usr/local/lib/python2.7/functools.py", line 33, in update_wrapper
    setattr(wrapper, attr, getattr(wrapped, attr))
AttributeError: 'functools.partial' object has no attribute '__module__'

When you create a function on the fly with functools.partial, it does not assign it a __module__ property, which is assigned when a function is built with the def keyword. @six.wraps (which is just an alias to functools.wraps) requires a __module__ property for aliasing the docstring.

You can get around it with:

# Make the partial that you were making before.
partial_func = functools.partial(func, *args, **kwargs)

# Assign it an appropriate __module__ property based on the function it wraps.
partial_func.__module__ = func.__module__

After that it should work with wraps.

[theacodes]

@lukesneeringer No need, we already fixed that in core. :)

[theacodes]

retry stuff LGTM