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.

Sign up for GitHub

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

Improve Ticket/Descriptor error messages for all gRPC usages #1174

Merged
merged 3 commits into from
Sep 1, 2021
Merged

Improve Ticket/Descriptor error messages for all gRPC usages #1174

merged 3 commits into from
Sep 1, 2021

Conversation

nbauernfeind
Copy link
Member

@nbauernfeind nbauernfeind commented Aug 31, 2021 •
edited
Loading

Fixes #1153, #1167

@nbauernfeind nbauernfeind added grpc NoDocumentationNeeded release blocker A bug/behavior that puts is below the "good enough" threshold to release. labels Aug 31, 2021
@nbauernfeind nbauernfeind added this to the Aug 2021 milestone Aug 31, 2021
@nbauernfeind nbauernfeind self-assigned this Aug 31, 2021
@nbauernfeind nbauernfeind linked an issue Aug 31, 2021 that may be closed by this pull request
Provide better error message to user when table API request's result_id is missing #1167
Closed
devinrsmith
devinrsmith previously approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Looks tedious, mostly mechanical. Would probably be good to break out strictly mechanical / refactoring work (logId) from the logical changes (catching when consoleId is empty, switch case on TableReference, etc) in the future.

niloc132
niloc132 previously approved these changes Aug 31, 2021
View reviewed changes
grpc-api/src/main/java/io/deephaven/grpc_api/appmode/ApplicationTicketResolver.java Outdated Show resolved Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/appmode/ApplicationTicketResolver.java Show resolved Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/session/ExportTicketResolver.java Show resolved Hide resolved
grpc-api/src/main/java/io/deephaven/grpc_api/session/TicketRouter.java
}
if (descriptor.getPathCount() <= 0) {
throw GrpcUtil.statusRuntimeException(Code.FAILED_PRECONDITION,
"Cannot find resolver: flight descriptor does not have route path");
"Could not resolve '" + logId + "': flight descriptor does not have route path");
}

final String route = descriptor.getPath(0);
final TicketResolver resolver = descriptorResolverMap.get(route);
if (resolver == null) {
throw GrpcUtil.statusRuntimeException(Code.FAILED_PRECONDITION,
Copy link
Member

Choose a reason for hiding this comment

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

any chance you want these to be NOT_FOUND instead? FAILED_PRECONDITION isn't inaccurate, but NOT_FOUND could make more sense with nonsense tickets, potentially

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel that NOT_FOUND implies that the ticket is structured correctly, but that it just doesn't exist. I've tried to use FAILED_PRECONDITION if the ticket is messed up or unparseable somehow.

@nbauernfeind
Copy link
Member Author

@devinrsmith while that often can make sense -- in this case I was driven by the API change (which you called refactoring, but I'm not sure this is a form of refactoring -- this is new functionality to provide better error messages). The call sites to resolve (which I had to fix anyways) were the proper places to identify opportunities for missing/empty protobuf fields.

@nbauernfeind nbauernfeind dismissed stale reviews from niloc132 and devinrsmith via 4235b91 August 31, 2021 22:12
@devinrsmith devinrsmith self-requested a review August 31, 2021 22:14
devinrsmith
devinrsmith previously approved these changes Aug 31, 2021
View reviewed changes
@devinrsmith devinrsmith self-requested a review September 1, 2021 00:15
@nbauernfeind nbauernfeind merged commit d79bed1 into deephaven:main Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devinrsmith devinrsmith devinrsmith approved these changes

@niloc132 niloc132 niloc132 left review comments

Assignees

@nbauernfeind nbauernfeind

Labels
grpc NoDocumentationNeeded release blocker A bug/behavior that puts is below the "good enough" threshold to release.
Projects
None yet
Milestone
Aug 2021
3 participants
@nbauernfeind @niloc132 @devinrsmith