Conversation

olavloite

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

@googlebotgooglebot added the cla: yesThis human has signed the Contributor License Agreement.label Feb 4, 2020
snehashah16
snehashah16 reviewed Feb 4, 2020
View reviewed changes

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

Choose a reason for hiding this comment

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

LGTM.

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

@olavloite

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-automationyoshi-automation mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on . Already have an account? Sign in to comment

@snehashah16 snehashah16 snehashah16 left review comments

@hengfengli hengfengli hengfengli approved these changes

@skuruppu skuruppu skuruppu approved these changes

No one assigned
cla: yesThis human has signed the Contributor License Agreement.
None yet
No milestone

Successfully merging this pull request may close these issues.