Skip to content

Move '_catch_remap_gax_error' to 'core.exceptions'. - #3444

Closed
tseaver wants to merge 4 commits into
googleapis:masterfrom
tseaver:3175-remap_gax_errors_uniformly
Closed

Move '_catch_remap_gax_error' to 'core.exceptions'.#3444
tseaver wants to merge 4 commits into
googleapis:masterfrom
tseaver:3175-remap_gax_errors_uniformly

Conversation

@tseaver

@tseavertseaver commented May 19, 2017

Copy link
Copy Markdown
Contributor

Toward #3175.

This is the first step toward applying a more uniform mapping of GAX error codes to our exception hierarchy. For a narrower, pubsub-only fix for #3175, see #3443.

@tseavertseaver added api: datastore Issues related to the Datastore API. api: core gax labels May 19, 2017
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2017
Comment threadcore/google/cloud/exceptions.py Outdated
_HAVE_GRPC = False
_Rendezvous = None
else:
_HAVE_GRPC = True

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

if code is not None:
_HTTP_CODE_TO_EXCEPTION[code] = _eklass

_GRPC_ERROR_MAPPING = {

This comment was marked as spam.

Comment threadcore/google/cloud/exceptions.py Outdated


@contextlib.contextmanager
def _catch_remap_gax_error():

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Comment threadcore/setup.py
'protobuf >= 3.0.0',
'google-auth >= 0.4.0, < 2.0.0dev',
'google-auth-httplib2',
'google-gax>=0.15.7, <0.16dev',

This comment was marked as spam.

This comment was marked as spam.

Comment threadcore/tests/unit/test_exceptions.py Outdated

import unittest

from google.cloud.exceptions import _HAVE_GRPC

This comment was marked as spam.

@dhermes

Copy link
Copy Markdown
Contributor

@lukesneeringer Are you good with the dependency changes?

@tseaver

Copy link
Copy Markdown
ContributorAuthor

Note that this change will require releasing a new version of google-cloud-core before / with updated API libraries which use the new google.cloud.exceptions._catch_remap_gax_error. It would obsolete #3443 (which is fine by me).

@dhermes

Copy link
Copy Markdown
Contributor

👍 on the release (I think @lukesneeringer is somewhat anti to the idea of google-cloud-core churn)

@tseaver

Copy link
Copy Markdown
ContributorAuthor

@dhermes was there another new core feature / fix you thought would land soon? Right now, I see three other PRs touching core since the core-0.24.1 release:

@dhermes

Copy link
Copy Markdown
Contributor

#3436 was what I was talking about. Looks like we've hit 24 hours, so LGTM?

@tseaver

Copy link
Copy Markdown
ContributorAuthor

@dhermes this PR wasn't in the penalty box :). I'm fine to merge today unless @lukesneeringer has issues with us pushing a new core release to support the subsequent API-specific remapping work.

@tseavertseaver mentioned this pull request Jun 7, 2017
@lukesneeringer

Copy link
Copy Markdown
Contributor

Is the core change backwards-compatible (from an API surface standpoint), or is it something where every single subpackage has to be updated?

I do not mind churn on core; back-incompat churn where everything has to move along with it makes me nervous.

@tseaver

Copy link
Copy Markdown
ContributorAuthor

@lukesneeringer the API surface changes are backward-compatible (the add a new, non-API helper function), but the dependency changes are maybe problematic (google-gax becomes an unconditional requirement for core).

@lukesneeringer

Copy link
Copy Markdown
Contributor

That is a smidge concerning. It would mean that APIs based on HTTP only would now require the installation of (but not use of) GRPC.

Would it be possible to only do the mapping if GAX was present, but not require it? (e.g. try / except ImportError)

@tseaver

Copy link
Copy Markdown
ContributorAuthor

That is a smidge concerning. It would mean that APIs based on HTTP only would now require the installation of (but not use of) GRPC.

We already have this for "dual mode" APIs: even on "classic" GAE they install grpc / gax.

Would it be possible to only do the mapping if GAX was present, but not require it? (e.g. try / except ImportError)

We can indeed revert to doing that: this PR actually backs that stuff out.

@theacodes

Copy link
Copy Markdown
Contributor

I vote with Luke - can we do this in a way that doesn't require gRPC in core (for now)? I would really like to have this so I can move #3617 forward.

@tseaver

Copy link
Copy Markdown
ContributorAuthor

Redoing this on a new branch (recent core changes make me leery of messing up the original one).

@tseaver

Copy link
Copy Markdown
ContributorAuthor

Reworked in #3707.

@tseaver
tseaver deleted the 3175-remap_gax_errors_uniformly branch August 12, 2017 17:29
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: coreapi: datastoreIssues related to the Datastore API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tseaver@dhermes@lukesneeringer@theacodes@googlebot