Wrap grpc status with exception #324
Conversation
This is part of the fixes for googleapis#119.
Instead of raising a plain std::runtime_error use a more detailed exception that derives from it. This fixes googleapis#119.
googlebot
added
the
cla: yes
coryan
requested review from
DanielMahu,
deepankarsharma,
garye,
hemant-qlogic,
manish-qlogic,
mbrukman,
richjames0 and
sduskis
Because the previous reformat was wrong, grrr.
|
|
||
| namespace bigtable { | ||
| inline namespace BIGTABLE_CLIENT_NS { | ||
| constexpr char const* kKnownCodes[] = { |
There was a problem hiding this comment.
Does grpc not have a way to convert error codes to their textual representation?
There was a problem hiding this comment.
None that I could find. There is none listed in the user documentation:
Nor in the code:
https://github.com/grpc/grpc/blob/master/include/grpcpp/impl/codegen/status_code_enum.h
https://github.com/grpc/grpc/blob/master/include/grpcpp/impl/codegen/status.h
| bigtable::GRpcError ex(msg, status); | ||
| std::cerr << "Aborting because exceptions are disabled: " << ex.what() | ||
| << std::endl; | ||
| std::abort(); |
There was a problem hiding this comment.
- Could you clarify the error handling strategy here from a users point of view?
- In the exceptions case will the exception be raised all the way to the user, or will we catch it and do the appropriate thing in the bigtable client codebase?
- What do we expect the user to do in the no exceptions case?
There was a problem hiding this comment.
The exceptions are raised all the way to the user. If the user compiled without exceptions the error "handling" is we abort. If the user is called "Google" there is an internal API to catch (most) errors without exceptions. This pending PR has more context:
| grpc::Status const& status); | ||
|
|
||
| private: | ||
| grpc::StatusCode error_code_; |
There was a problem hiding this comment.
In case this exception will reach users drop usage of grpc::StatusCode and change name away from GRpcError.
There was a problem hiding this comment.
Why? The error represents a run-time problem, reported by gRPC, that stops the library from completing its work. Is not like we want (or could if we wanted to) hide the fact that gRPC is the underlying RPC mechanism.
|
Let me know if those answers make sense. Happy to discuss in person if the github reviews do not provide enough bandwidth. |
Thanks to @deepankarsharma for the ideas.
|
Per our offline discussion, I updated the documentation and created a bug for that idea around making the call to |
3 participants
This fixes #119. Instead of raising a plain
std::runtime_errorexception with the information in the
what()string thelibrary now raises
bigtble::GRpcErrorwith the relevantgrpc::Statusfields (error code, error message, and error details).The class is derived from
std::runtime_errorbecause that isthe exception that one should use to, well, represent run-time
errors, which these are.