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.

Sign up for GitHub

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

Jump to bottom

Add More Retries to 400 on Query #2412

Merged
merged 1 commit into from
Sep 24, 2016
Merged

Add More Retries to 400 on Query #2412

merged 1 commit into from
Sep 24, 2016

Conversation

waprin
Copy link
Contributor

@waprin waprin commented Sep 23, 2016 •
edited by dhermes
Loading

Fixes #2402.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 23, 2016
@waprin
Copy link
Contributor Author

waprin commented Sep 23, 2016

@dhermes Can't do much about all the retrying but I think this will fix the test.

dhermes
dhermes reviewed Sep 23, 2016
View reviewed changes
system_tests/monitoring.py
@@ -26,6 +26,7 @@

retry_404 = RetryErrors(NotFound, max_tries=5)
retry_404_500 = RetryErrors((NotFound, InternalServerError))
retry_400_500 = RetryErrors((BadRequest, InternalServerError))
retry_500 = RetryErrors(InternalServerError)
retry_503 = RetryErrors(ServiceUnavailable)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Sep 23, 2016

Thanks for the quick turnaround @waprin!

@dhermes
Copy link
Contributor

dhermes commented Sep 23, 2016

Can't do much about all the retrying but I think this will fix the test.

You can tweak the retry defaults like the initial delay and the number of retries.

@dhermes
Copy link
Contributor

dhermes commented Sep 23, 2016

@waprin This doesn't fix the issue reported in #2402.

I just checked out the stacktrace and it happens when the iterator is cast to a list: https://github.com/GoogleCloudPlatform/google-cloud-python/blob/af4f346e238e1a45ff5754335b9f8a4d98708e49/system_tests/monitoring.py#L212

@waprin waprin changed the title Add 400 Retry to Monitoring Write Point System Test Add More Retries to 400 on Query Sep 23, 2016
@waprin
Copy link
Contributor Author

waprin commented Sep 23, 2016

@dhermes does it look right to you now?

dhermes
dhermes reviewed Sep 23, 2016
View reviewed changes
system_tests/monitoring.py
@@ -206,7 +206,7 @@ def _has_timeseries(result):
return len(list(result)) > 0
retry_result = RetryResult(_has_timeseries, max_tries=7)(
client.query)
return RetryErrors(BadRequest)(retry_result)
return RetryErrors(BadRequest, max_tries=7)(retry_result)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Sep 23, 2016

LGTM pending Travis.

Can you

  1. Ping me once Travis goes green (I'll merge for you)
  2. File an issue to make the blunt instrument sharper (i.e. blunt thing == these error retries)

@waprin
Copy link
Contributor Author

waprin commented Sep 24, 2016

@dhermes travis green

@dhermes dhermes merged commit 5edcaaf into googleapis:master Sep 24, 2016
@tseaver tseaver added api: monitoring Issues related to the Cloud Monitoring API. flaky labels Sep 24, 2016
@tseaver tseaver mentioned this pull request Sep 24, 2016
Closed
@dhermes dhermes mentioned this pull request Sep 29, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhermes dhermes dhermes left review comments

Assignees
No one assigned
Labels
api: monitoring Issues related to the Cloud Monitoring API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@waprin @dhermes @tseaver @googlebot