Add a flag and some tests for returning RPC errors in RPC responses

go/mysql/proto/rpcerror_bson.go

@@ -19,7 +19,7 @@ func (rPCError *RPCError) MarshalBson(buf *bytes2.ChunkedWriter, key string) {
 	bson.EncodeOptionalPrefix(buf, bson.Object, key)
 	lenWriter := bson.NewLenWriter(buf)
 
-	bson.EncodeInt(buf, "Code", rPCError.Code)
+	bson.EncodeInt64(buf, "Code", rPCError.Code)
 	bson.EncodeString(buf, "Message", rPCError.Message)
 
 	lenWriter.Close()
@@ -40,7 +40,7 @@ func (rPCError *RPCError) UnmarshalBson(buf *bytes.Buffer, kind byte) {
 	for kind := bson.NextByte(buf); kind != bson.EOO; kind = bson.NextByte(buf) {
 		switch bson.ReadCString(buf) {
 		case "Code":
-			rPCError.Code = bson.DecodeInt(buf, kind)
+			rPCError.Code = bson.DecodeInt64(buf, kind)
 		case "Message":
 			rPCError.Message = bson.DecodeString(buf, kind)
 		default:

go/mysql/proto/structs.go

@@ -91,7 +91,7 @@ type QueryResult struct {
 // RPCError is the structure that is returned by each RPC call, which contains
 // the error information for that call.
 type RPCError struct {
-	Code    int
+	Code    int64
 	Message string
 }
 

go/vt/tabletserver/gorpcqueryservice/sqlquery.go

@@ -5,6 +5,8 @@
 package gorpcqueryservice
 
 import (
+	"flag"
+
 	mproto "github.com/youtube/vitess/go/mysql/proto"
 	"github.com/youtube/vitess/go/vt/callinfo"
 	"github.com/youtube/vitess/go/vt/servenv"
@@ -14,6 +16,10 @@ import (
 	"golang.org/x/net/context"
 )
 
+var (
+	rpcErrorOnlyInReply = flag.Bool("rpc-error-only-in-reply", false, "if true, supported RPC calls will only return errors as part of the RPC server response")
+)
+
 // SqlQuery is the server object for gorpc SqlQuery
 type SqlQuery struct {
 	server queryservice.QueryService
@@ -48,7 +54,10 @@ func (sq *SqlQuery) Execute(ctx context.Context, query *proto.Query, reply *mpro
 	defer sq.server.HandlePanic(&err)
 	execErr := sq.server.Execute(callinfo.RPCWrapCallInfo(ctx), query, reply)
 	tabletserver.AddTabletErrorToQueryResult(execErr, reply)
-	return nil
+	if *rpcErrorOnlyInReply {
+		return nil
+	}
+	return execErr
 }
 
 // StreamExecute is exposing tabletserver.SqlQuery.StreamExecute

go/vt/tabletserver/tablet_error.go

@@ -223,7 +223,7 @@ func AddTabletErrorToQueryResult(err error, reply *mproto.QueryResult) {
 	if ok {
 		rpcErr = mproto.RPCError{
 			// Transform TabletError code to VitessError code
-			Code: terr.ErrorType + vterrors.TabletError,
+			Code: int64(terr.ErrorType) + vterrors.TabletError,
 			// Make sure the the VitessError message is identical to the TabletError
 			// err, so that downstream consumers will see identical messages no matter
 			// which endpoint they're using.

go/vt/tabletserver/tabletconntest/tabletconntest.go

@@ -22,6 +22,7 @@ import (
 // FakeQueryService has the server side of this fake
 type FakeQueryService struct {
 	t                        *testing.T
+	hasError                 bool
 	panics                   bool
 	streamExecutePanicsEarly bool
 }
@@ -170,7 +171,11 @@ func (f *FakeQueryService) Execute(ctx context.Context, query *proto.Query, repl
 	if query.TransactionId != executeTransactionId {
 		f.t.Errorf("invalid Execute.Query.TransactionId: got %v expected %v", query.TransactionId, executeTransactionId)
 	}
-	*reply = executeQueryResult
+	if f.hasError {
+		*reply = executeQueryResultError
+	} else {
+		*reply = executeQueryResult
+	}
 	return nil
 }
 
@@ -207,6 +212,13 @@ var executeQueryResult = mproto.QueryResult{
 	},
 }
 
+var executeQueryResultError = mproto.QueryResult{
+	Err: mproto.RPCError{
+		Code:    1000,
+		Message: "succeeded despite err",
+	},
+}
+
 func testExecute(t *testing.T, conn tabletconn.TabletConn) {
 	t.Log("testExecute")
 	ctx := context.Background()
@@ -219,6 +231,19 @@ func testExecute(t *testing.T, conn tabletconn.TabletConn) {
 	}
 }
 
+func testExecuteError(t *testing.T, conn tabletconn.TabletConn) {
+	t.Log("testExecuteError")
+	ctx := context.Background()
+	_, err := conn.Execute(ctx, executeQuery, executeBindVars, executeTransactionId)
+	if err == nil {
+		t.Fatalf("Execute was expecting an error, didn't get one")
+	}
+	expectedErr := "vttablet: succeeded despite err"
+	if err.Error() != expectedErr {
+		t.Errorf("Unexpected error from Execute: got %v wanted %v", err, expectedErr)
+	}
+}
+
 func testExecutePanics(t *testing.T, conn tabletconn.TabletConn) {
 	ctx := context.Background()
 	if _, err := conn.Execute(ctx, executeQuery, executeBindVars, executeTransactionId); err == nil || !strings.Contains(err.Error(), "caught test panic") {
@@ -531,6 +556,11 @@ func TestSuite(t *testing.T, conn tabletconn.TabletConn, fake *FakeQueryService)
 	testExecuteBatch(t, conn)
 	testSplitQuery(t, conn)
 
+	// fake should return an error, make sure errors are handled properly
+	fake.hasError = true
+	testExecuteError(t, conn)
+	fake.hasError = false
+
 	// force panics, make sure they're caught
 	fake.panics = true
 	testBeginPanics(t, conn)

go/vt/vterrors/vterrors.go

@@ -26,7 +26,7 @@ const (
 // VitessError is the error type that we use internally for passing structured errors
 type VitessError struct {
 	// Error code of the Vitess error
-	Code int
+	Code int64
 	// Additional context string, distinct from the error message. For example, if
 	// you wanted an error like "foo error: original error", the Message string
 	// should be "foo error: "
@@ -53,7 +53,6 @@ func (e *VitessError) AsString() string {
 // FromRPCError recovers a VitessError from an RPCError (which is how VitessErrors
 // are transmitted across RPC boundaries).
 func FromRPCError(rpcErr mproto.RPCError) error {
-	fmt.Printf("RPCError: %v\n", rpcErr)
 	if rpcErr.Code == 0 && rpcErr.Message == "" {
 		return nil
 	}
@@ -63,8 +62,8 @@ func FromRPCError(rpcErr mproto.RPCError) error {
 	}
 }
 
-// AddPrefix allows a string to be prefixed to an error, without nesting a new VitessError.
-func AddPrefix(prefix string, in error) error {
+// WithPrefix allows a string to be prefixed to an error, without nesting a new VitessError.
+func WithPrefix(prefix string, in error) error {
 	vtErr, ok := in.(*VitessError)
 	if !ok {
 		return fmt.Errorf("%s: %s", prefix, in)

test/tablet.py

@@ -484,6 +484,8 @@ class Tablet(object):
     if supports_backups:
       args.extend(['-restore_from_backup'] + get_backup_storage_flags())
 
+    args.extend(['-rpc-error-only-in-reply=true'])
+
     if extra_args:
       args.extend(extra_args)