-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add exception handler for ACL resource exhaustion case #1887
Add exception handler for ACL resource exhaustion case #1887
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - It makes sense that we need to do this per snapshot create type and I like the shared utility. Glad to take another look after tests are added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once tests pass, lgtm 👍
…ich is handled by the global exception handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought, but otherwise LGTM!
As we talked about in standup, if there a lower overhead way to add a unit test, I think that would be good, but not necessary to merge in my opinion.
…n type to avoid adding another exception clause to the global exception handler
…porting' into ps/dt-1097-better-quota-error-reporting
Test - remove exception handler code
|
Jira ticket: https://broadworkbench.atlassian.net/browse/DT-1097
Addresses
If a user hits a BigQuery quota when creating a snapshot, the error returned doesn't include information about the underlying problem or how to fix it. This change adds that information to the flight's error response.
Summary of changes
In the case where this problem occurs, a
GoogleResourceException
is thrown with a wrapped cause ofBigQueryException
with the messageToo many authorized entities in this dataset. The maximum number of authorized views, routines, and datasets combined is 2500.
. The code checks for this case and returns a custom exception. When the user later retrieves the flight result, they'll get a400
response and see the custom error message.Testing Strategy