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

Refactor 'gcloud.streaming.http_wrapper' exception handling #1249

Merged
merged 3 commits into from
Nov 30, 2015
Merged

Refactor 'gcloud.streaming.http_wrapper' exception handling #1249

merged 3 commits into from
Nov 30, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 30, 2015

  • Catch only explicitly retryable exceptions, and handle retry count / interval explicitly in make_api_request.
  • Delete the handle_heep_exceptions helper, and the retry_func argument which used it as a default.
  • Rename _rebuild_http_connections -> _reset_http_connections for clarity.

Closes #1223.

- Catch only explicitly retryable exceptions, and handle retry count /
  interval explicitly in 'make_api_request'.

- Delete the 'handle_heep_exceptions' helper, and the 'retry_func'
  argument which used it as a default.

- Rename '_rebuild_http_connections' -> '_reset_http_connections' for
  clarity.

Closes #1223.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 30, 2015
@dhermes
Copy link
Contributor

dhermes commented Nov 30, 2015

Happy to see this! I had to peek into this mess a bit for #1248. If they collide for merging, let's stick with this one first.

@@ -399,11 +345,23 @@ def _make_api_request_no_retry(http, http_request, redirections=5,
return response


_RETRYABLE_EXCEPTIONS = (

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Nov 30, 2015

Wow this was so much less work than I expected. w00t

@dhermes
Copy link
Contributor

dhermes commented Nov 30, 2015

LGTM

@tseaver
Copy link
Contributor Author

tseaver commented Nov 30, 2015

@dhermes I will merge with pylint failing on Travis, since we are still working out #1248.

@dhermes
Copy link
Contributor

dhermes commented Nov 30, 2015

OK. The new commit since I reviewed is due to the ExceptionType as exc keeping exc limited to the except block's scope in Py3k?

dhermes added a commit that referenced this pull request Nov 30, 2015
…on_handling

Refactor 'gcloud.streaming.http_wrapper' exception handling
@dhermes dhermes merged commit bc181db into googleapis:master Nov 30, 2015
@dhermes
Copy link
Contributor

dhermes commented Nov 30, 2015

@tseaver Please answer my above question, but I went ahead and merged so I can re-do #1248 so we can get it merged.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 30, 2015

The new commit since I reviewed is due to the ExceptionType as exc keeping exc limited to the except block's scope in Py3k?

Yes: it provoked an UnboundLocalError.

@tseaver tseaver deleted the 1223-refactor_streaming_exception_handling branch November 30, 2015 19:54
@dhermes
Copy link
Contributor

dhermes commented Nov 30, 2015

OK Good deal. 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.

3 participants