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

fix: address issues with concurrent BigQuery tests #3426

Merged

Conversation

shollyman
Copy link
Contributor

This PR changes how testing is done for the BQ tests which now run multiple instances concurrently.

Basic premise is we inject new values into the code under test, and use the template syntax to elide the replacement from the embedded representations. the test harness takes care of deleting resources.

Gotchas:
* Keeping track of which sample tags are in scope at a given variable is
a pain.

* variables aren't all instantiated at the same spot, so you need to
leverage the END/START tag blocks for each variable that gets overriden.
@shollyman shollyman requested a review from a team as a code owner April 17, 2020 21:45
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 17, 2020
@leahecole
Copy link
Collaborator

I'm here for it, but I tend to have less strong opinions about this than some of the other folks. @tmatsuo has done a LOT of refactoring tests recently, so I want his eyes too to double check

Copy link
Contributor

@tmatsuo tmatsuo left a comment

Choose a reason for hiding this comment

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

Looks good overall.

If the client library doesn't automatically retry transient failures, I'd retry few times upon such exceptions.
I found that backoff is a decent library for this purpose.

The code looks like:

    @backoff.on_exception(backoff.expo, max_time=60)
    def delete_dataset():
        try:
            client.delete_dataset(item, delete_contents=True)
        except NotFound as e:
            # We ignore this case.                                                                                                                                 
            print("The dataset doesn't exist: detail: {}".format(str(e)))
    delete_dataset()

If the client library already retries transient errors, just ignore my comment. Also, I'm open to do that research after merging this PR.

@@ -14,6 +14,7 @@

from google.cloud import bigquery
import pytest
import uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do the following?

import uuid

from google.cloud import bigquery
import pytest

import authorized_view_tutorial

Usually imports should be 1) standard libs 2) 3rd party libs and 3) package local libs, separated by an empty line.

bigquery/cloud-client/quickstart_test.py Outdated Show resolved Hide resolved
@shollyman
Copy link
Contributor Author

Looks good overall.

If the client library doesn't automatically retry transient failures, I'd retry few times upon such exceptions.
I found that backoff is a decent library for this purpose.

The code looks like:

    @backoff.on_exception(backoff.expo, max_time=60)
    def delete_dataset():
        try:
            client.delete_dataset(item, delete_contents=True)
        except NotFound as e:
            # We ignore this case.                                                                                                                                 
            print("The dataset doesn't exist: detail: {}".format(str(e)))
    delete_dataset()

If the client library already retries transient errors, just ignore my comment.

Yeah, BQ uses a default retry policy.

https://googleapis.dev/python/bigquery/latest/generated/google.cloud.bigquery.retry.DEFAULT_RETRY.html

Copy link
Contributor

@tmatsuo tmatsuo left a comment

Choose a reason for hiding this comment

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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