For discussion: rework error plumbing when proto.Call is involved #1403
Comments
|
I don't think the first option is really there because it means that any error must be a proto error, on top of its other drawbacks. In the second option, |
|
What do you mean by "unwrapping the One other consideration is batches: in a batch, each response carries its own error (and the batch has a whole has an error which is defined to be the first non-nil error in the batch). This is more difficult to express with returned errors than with errors in the header (although maybe batches can be treated as something like a transport boundary) |
I meant that the
I think this is orthogonal. We would treat each request/response in the batch as described, and do the batch special magic in a central place (process boundary anyway). |
|
But proto.Response is not currently a wrapper; it's an interface implemented by all the concrete response type. We don't currently have a type corresponding to the headerless message. Are you proposing a refactoring where proto.Response is no longer an interface but a concrete message containing a ResponseHeader and a byte array (representing an encoded headerless response of an unspecified type)? |
|
The end result should be that we can decompose the equivalent of today's |
|
Could simply have split |
|
|
|
@bdarnell I think the point of |
|
Yes, but how does that marker interface help? Any method that takes a |
|
I was thinking that |
|
So you would change the protos themselves so that |
|
Yeah, I think that's the only way to get the properties described here |
|
I think the proposal is to have Response contain a Header, and a oneof with all the different types, where all the different types themselves have a KVHeader that contains the keys, etc and not the error. That's a pretty big change. Another idea is to just enforce that error is not set at the boundary in our code. Not enforced by the language but an easier change to make. |
tamird
added
the
C-cleanup
|
Update on this: the only remaining non-conformances are $ git grep -F 'proto.Call' | grep -F func | grep -vF _test
client/batch.go:func (b *Batch) InternalAddCall(call proto.Call) {
client/db.go:func (db *DB) send(calls ...proto.Call) (err error) {
client/http_sender.go:func (s *httpSender) Send(_ context.Context, call proto.Call) {
client/rpc/rpc_sender.go:func (s *Sender) Send(_ context.Context, call proto.Call) {
client/sender.go:type SenderFunc func(context.Context, proto.Call)
client/sender.go:func (f SenderFunc) Send(ctx context.Context, c proto.Call) {
client/txn.go:func (ts *txnSender) Send(ctx context.Context, call proto.Call) {
client/txn.go:func endTxnCall(commit bool) proto.Call {
client/txn.go:func (txn *Txn) send(calls ...proto.Call) error {
client/txn.go:func (txn *Txn) updateState(calls []proto.Call) {
kv/dist_sender.go:func (ds *DistSender) getDescriptors(call proto.Call) (*proto.RangeDescriptor, *proto.RangeDescriptor, error) {
kv/dist_sender.go:func (ds *DistSender) Send(ctx context.Context, call proto.Call) {
kv/local_sender.go:func (ls *LocalSender) Send(ctx context.Context, call proto.Call) {
kv/txn_coord_sender.go:func (tc *TxnCoordSender) Send(ctx context.Context, call proto.Call) {
kv/txn_coord_sender.go:func (tc *TxnCoordSender) sendOne(ctx context.Context, call proto.Call) { |
|
@tschottdorf we might want to extend this issue to the removal of errors from non-batch responses; what do you think? |
|
Yes, I think that's a good idea.
|
We currently have an inconsistent error handling story where
proto.Responseis concerned (usually viaproto.Call). Some grep of all the functions which handle these arguments (excluding tests) is below. Note that many of these have an explicit error return.Some methods return the error outright, some set it on the
proto.Responsedirectly, and some do a mix of the two. We should resolve this inconsistency because it leads to confusing code and a very high probability of subtle bugs. There are two approaches we could take:standardize on
ResponseHeader.SetGoError()This requires that we remove error returns from all methods that operate on
proto.Responses. In addition, it will mean that we will need to remember to check.GoError()each time we call such a method. This is fragile, and we will have no tool support.standardize on error returns
This requires that we create a new structure to be obtained by unwrapping the
proto.Responsewhich will not be able to carry an error. All methods which currently operate onproto.Response(with the exception of those that live at process boundaries) will need to change to return explicit errors rather than attaching errors directly to theirproto.Responseargument.This option is my preference. It provides for tooling support via errcheck and is more idiomatic. That being said, the floor is open. Please 🚲 🏡 !
The text was updated successfully, but these errors were encountered: