Remove error return value from ExecuteCmd #1400
Conversation
| @@ -123,6 +123,12 @@ func (ls *LocalSender) Send(ctx context.Context, call proto.Call) { | |||
| var err error | |||
| var store *storage.Store | |||
|
|
|||
| defer func() { | |||
| if err != nil { | |||
There was a problem hiding this comment.
This pattern seems fragile: if err is ever shadowed then the error wouldn't be returned correctly. I'd rather call SetGoError each time we have if err != nil { return } instead of calling it in a defer.
This is already returned on the Call's reply header.
| pushType := proto.PUSH_TIMESTAMP | ||
| if proto.IsWrite(args) { | ||
| pushType = proto.ABORT_TXN | ||
| if err = rng.AddCmd(ctx, proto.Call{Args: args, Reply: reply}, true); err != nil { |
There was a problem hiding this comment.
rng.AddCmd() has the same issue as this method. Does rng.AddCmd() ever return an error that is different from reply.GoError? If so, we used to leave the reply.GoError alone but this change will overwrite it with the returned error. I'm not sure if that has anything to do with the test failure but I'm having a hard time spotting other behavioral changes.
|
I haven't looked at the details so it may not be relevant here, but something that I tripped over in the past is that Btw, CircleCI has a bunch of test failures - which one should I be looking at? |
|
Closing this in favour of the approach discussed in #1403 |
- always update response cache in applyRaftCommand, allowing early returns - add replicaCorruptionError, bogus handling and test - anticipate cockroachdb#1400 by not using reply.Header().GoError() as authoritative error source and by setting it only in r.processRaftCommand (though it is still set in r.executeCmd and below) for now - deduplicate some logic around the verification of leader leases - move leadership check from applyRaftCommand to processRaftCommand and add a comment
- always update response cache in applyRaftCommand, allowing early returns - add replicaCorruptionError, bogus handling and test - anticipate cockroachdb#1400 by not using reply.Header().GoError() as authoritative error source and by setting it only in r.processRaftCommand (though it is still set in r.executeCmd and below) for now - deduplicate some logic around the verification of leader leases - move leadership check from applyRaftCommand to processRaftCommand and add a comment
This is already returned on the Call's reply header.
This refactor introduced a test failure (see the first commit which fixes the error message for that test), but I'm not sure why.