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

Jump to bottom

fix: use resource type to identify type of error #57

Merged
merged 2 commits into from Feb 5, 2020
Merged

fix: use resource type to identify type of error #57

merged 2 commits into from Feb 5, 2020

Conversation

olavloite
Copy link
Collaborator

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 4, 2020
snehashah16
snehashah16 reviewed Feb 4, 2020
View reviewed changes
Copy link
Contributor

@snehashah16 snehashah16 left a comment

Choose a reason for hiding this comment

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

dont block on my approval.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java
} 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

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
Collaborator Author

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.

hengfengli
hengfengli approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

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

snehashah16 commented Feb 5, 2020 via email

@olavloite
Copy link
Collaborator Author

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.

@yoshi-automation yoshi-automation mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snehashah16 snehashah16 snehashah16 left review comments

@hengfengli hengfengli hengfengli approved these changes

@skuruppu skuruppu skuruppu approved these changes

Assignees
No one assigned
Labels
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.

None yet
5 participants
@olavloite @snehashah16 @hengfengli @skuruppu @googlebot