-
Notifications
You must be signed in to change notification settings - Fork 80
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
Improve Ticket/Descriptor error messages for all gRPC usages #1174
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.
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.
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
} | ||
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, |
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.
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
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.
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.
@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 |
4235b91
47a034b
to
4235b91
Compare
Fixes #1153, #1167