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

Playing with retries. #2040

Merged
merged 6 commits into from
Aug 2, 2016
Merged

Conversation

daspecster
Copy link
Contributor

So I was playing with getting the retries working on the bigquery system3 tests.

I'm not sure what fixtures need to be created to get all the tests passing but this seemed to get the bigquery tests to pass.

Also, I have the retry set pretty long, looks like it takes about an average of 20 seconds for it to eventually be consistent.

@daspecster daspecster added testing api: bigquery Issues related to the BigQuery API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Aug 1, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 1, 2016
from functools import wraps


def retry(ExceptionToCheck, tries=4, delay=3, backoff=2, logger=None):

This comment was marked as spam.

@daspecster
Copy link
Contributor Author

Sorry, I should have put more in the description I think.

So my point with this, was that there are a lot more errors after the main blocking bigquery error that we keep seeing. I'm sure this isn't the best solution.

When I run system-tests I get a bunch of datastore failures/errors. Are there some fixtures I need to setup to get those to work?

I can fix this up so we can merge and go from there though.

@dhermes
Copy link
Contributor

dhermes commented Aug 1, 2016

There is nothing wrong with the retry method, it just looks copy-pasted. You'd probably rather use it inside the test case than on the test case:

def test_case_for_thing(self):
    # do some set-up
    @retry(...)
    def thing_that_is_eventually_consistent():
        global_object.some_method(args)

    thing_that_is_eventually_consistent()

@daspecster
Copy link
Contributor Author

@dhermes LMKWYT.

@daspecster daspecster removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 1, 2016

@retry(Forbidden, tries=3, delay=30)
def update_dataset():
dataset.update()

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Aug 1, 2016

Thanks for taking the initiative here.

Searching
https://github.com/GoogleCloudPlatform/gcloud-python/search?q=eventual+consistency&type=Issues
https://github.com/GoogleCloudPlatform/gcloud-python/search?q=eventually+consistent&type=Issues
we should unify all the issues / code that have consistency implications.

In particular, we should discuss this in #1619


# We should try and keep `delay` and tries as low as possible to
# reduce test running time. tries=3 and delay=30 worked consistenly
# when updating a dataset.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@daspecster
Copy link
Contributor Author

@dhermes here's the class version of the retry decorator. LMWYT.

exception = None
tries = None
delay = None
backoff = None

This comment was marked as spam.


time.sleep(self.delay)
tries_counter -= 1
self.delay *= self.backoff

This comment was marked as spam.

This comment was marked as spam.


# We need to wait for the changes in the dataset to propgate and be
# eventually consistent. The alternative outcome is a 403 Forbidden
# response from upstream.

This comment was marked as spam.

tries_counter = self.tries
exception = self.exception
delay = self.delay
backoff = self.backoff

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Aug 2, 2016

LGTM pending Travis

@daspecster daspecster merged commit 2724805 into googleapis:master Aug 2, 2016
This was referenced Aug 3, 2016
@dhermes dhermes mentioned this pull request Aug 3, 2016
@daspecster daspecster deleted the retry-system3-tests branch January 24, 2017 15:18
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. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants