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: use resource type to identify type of error #57

Merged
merged 2 commits into from Feb 5, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Feb 4, 2020

Use the returned resource type instead of the error message to determine the type of error.

@googlebot googlebot added the cla: yes label Feb 4, 2020
Copy link
Contributor

@snehashah16 snehashah16 left a comment

dont block on my approval.

} else if (message != null && DATABASE_NOT_FOUND_MSG_PATTERN.matcher(message).matches()) {
return new DatabaseNotFoundException(token, message, cause);
ResourceInfo resourceInfo = extractResourceInfo(cause);
if (resourceInfo != null) {
Copy link
Contributor

@snehashah16 snehashah16 Feb 4, 2020

Choose a reason for hiding this comment

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

what about Instance Not Found ?

Copy link
Contributor Author

@olavloite olavloite Feb 5, 2020

Choose a reason for hiding this comment

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

The client library does not currently handle InstanceNotFound specifically, so it is currently not recognized as a separate error. I'll open a separate issue for it, as it could cause the same problems as DatabaseNotFound if someone deletes an instance while the client library has an active connection with a database on that instance.

Copy link
Contributor

@hengfengli hengfengli left a comment

LGTM.

@olavloite olavloite merged commit 89c3e77 into master Feb 5, 2020
12 checks passed
@olavloite olavloite deleted the use-resource-type-for-errors branch Feb 5, 2020
@snehashah16
Copy link
Contributor

@snehashah16 snehashah16 commented Feb 5, 2020

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Feb 5, 2020

May be worth considering, what if the resource info is not populated ? What's the fallback in that case ?

If the resource info is not populated, or it is populated with unknown values, the client library will automatically fall back to considering the error as a generic SpannerException. The effect of this would be that the client library will not stop sending RPCs for the database client that caused the error. The user will not really notice any difference, as the error is also returned to the user in both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants