vterrors: addres review comments (#2593)

Addressed review comments. I've changed the error codes for
cases we've agreed on. I've left the more contentious ones
unchanged for now.

BUG=32851872

go/mysqlconn/constants.go

@@ -350,7 +350,10 @@ func IsNum(typ uint8) bool {
 func IsConnErr(err error) bool {
  if sqlErr, ok := err.(*sqldb.SQLError); ok {
  num := sqlErr.Number()
- // Don't count query kill as connection error.
+ // ServerLost means that the query has already been
+ // received by MySQL and may have already been executed.
+ // Since we don't know if the query is idempotent, we don't
+ // count this error as connection error which could be retried.
  if num == CRServerLost {
  return false
  }

go/mysqlconn/constants_test.go

@@ -12,16 +12,20 @@ func TestIsConnErr(t *testing.T) {
  in error
  want bool
  }{{
- in: errors.New("t"),
+ in: errors.New("t"),
+ want: false,
  }, {
- in: sqldb.NewSQLError(5, "", ""),
+ in: sqldb.NewSQLError(5, "", ""),
+ want: false,
  }, {
  in: sqldb.NewSQLError(CRServerGone, "", ""),
  want: true,
  }, {
- in: sqldb.NewSQLError(CRServerLost, "", ""),
+ in: sqldb.NewSQLError(CRServerLost, "", ""),
+ want: false,
  }, {
- in: sqldb.NewSQLError(CRCantReadCharset, "", ""),
+ in: sqldb.NewSQLError(CRCantReadCharset, "", ""),
+ want: false,
  }}
  for _, tcase := range testcases {
  got := IsConnErr(tcase.in)

go/vt/tabletserver/connpool/pool.go

@@ -19,7 +19,7 @@ import (
 )
 
 // ErrConnPoolClosed is returned when the connection pool is closed.
-var ErrConnPoolClosed = vterrors.New(vtrpcpb.Code_UNAVAILABLE, "connection pool is closed")
+var ErrConnPoolClosed = vterrors.New(vtrpcpb.Code_INTERNAL, "internal error: unexpected: conn pool is closed")
 
 // usedNames is for preventing expvar from panicking. Tests
 // create pool objects multiple time. If a name was previously

go/vt/tabletserver/query_engine.go

@@ -293,7 +293,7 @@ func (qe *QueryEngine) GetPlan(ctx context.Context, logStats *tabletenv.LogStats
  r, err := conn.Exec(ctx, sql, 1, true)
  logStats.AddRewrittenSQL(sql, start)
  if err != nil {
- return nil, vterrors.Errorf(vtrpcpb.Code_UNKNOWN, "error fetching fields: %v", err)
+ return nil, err
  }
  plan.Fields = r.Fields
  }
@@ -320,7 +320,7 @@ func (qe *QueryEngine) GetStreamPlan(sql string) (*ExecPlan, error) {
  splan, err := planbuilder.GetStreamExecPlan(sql, GetTable)
  if err != nil {
  // TODO(sougou): Inspect to see if GetStreamExecPlan can return coded error.
- return nil, vterrors.New(vtrpcpb.Code_UNKNOWN, err.Error())
+ return nil, vterrors.New(vtrpcpb.Code_INVALID_ARGUMENT, err.Error())
  }
  plan := &ExecPlan{ExecPlan: splan, Table: table}
  plan.Rules = qe.queryRuleSources.filterByPlan(sql, plan.PlanID, plan.TableName.String())

go/vt/tabletserver/queryservice/wrapped.go

@@ -50,13 +50,15 @@ func canRetry(ctx context.Context, err error) bool {
  if err == nil {
  return false
  }
+
  select {
  case <-ctx.Done():
  return false
  default:
  }
+
  switch vterrors.Code(err) {
- case vtrpcpb.Code_UNAVAILABLE, vtrpcpb.Code_FAILED_PRECONDITION, vtrpcpb.Code_RESOURCE_EXHAUSTED:
+ case vtrpcpb.Code_UNAVAILABLE, vtrpcpb.Code_FAILED_PRECONDITION:
  return true
  }
  return false

go/vt/tabletserver/tabletconn/grpc_error.go

@@ -16,7 +16,7 @@ func ErrorFromGRPC(err error) error {
  if err == nil || err == io.EOF {
  return nil
  }
- return vterrors.New(vtrpcpb.Code(grpc.Code(err)), "vttablet: "+err.Error())
+ return vterrors.Errorf(vtrpcpb.Code(grpc.Code(err)), "vttablet: %v", err)
 }
 
 // ErrorFromVTRPC converts a *vtrpcpb.RPCError to vtError for
@@ -29,5 +29,5 @@ func ErrorFromVTRPC(err *vtrpcpb.RPCError) error {
  if code == vtrpcpb.Code_OK {
  code = vterrors.LegacyErrorCodeToCode(err.LegacyCode)
  }
- return vterrors.New(code, "vttablet: "+err.Message)
+ return vterrors.Errorf(code, "vttablet: %s", err.Message)
 }

go/vt/tabletserver/tabletenv/tabletenv.go

@@ -15,6 +15,7 @@ import (
  "github.com/youtube/vitess/go/stats"
  "github.com/youtube/vitess/go/tb"
  "github.com/youtube/vitess/go/vt/callerid"
+ vtrpcpb "github.com/youtube/vitess/go/vt/proto/vtrpc"
  "github.com/youtube/vitess/go/vt/sqlparser"
 )
 
@@ -29,10 +30,27 @@ var (
  WaitStats = stats.NewTimings("Waits")
  // KillStats shows number of connections being killed.
  KillStats = stats.NewCounters("Kills", "Transactions", "Queries")
- // InfoErrors shows number of various non critical errors happened.
- InfoErrors = stats.NewCounters("InfoErrors", "Retry", "DupKey")
  // ErrorStats shows number of critial erros happened.
- ErrorStats = stats.NewCounters("Errors", "Fail", "TxPoolFull", "NotInTx", "Deadlock", "Fatal")
+ ErrorStats = stats.NewCounters(
+ "Errors",
+ vtrpcpb.Code_OK.String(),
+ vtrpcpb.Code_CANCELED.String(),
+ vtrpcpb.Code_UNKNOWN.String(),
+ vtrpcpb.Code_INVALID_ARGUMENT.String(),
+ vtrpcpb.Code_DEADLINE_EXCEEDED.String(),
+ vtrpcpb.Code_NOT_FOUND.String(),
+ vtrpcpb.Code_ALREADY_EXISTS.String(),
+ vtrpcpb.Code_PERMISSION_DENIED.String(),
+ vtrpcpb.Code_UNAUTHENTICATED.String(),
+ vtrpcpb.Code_RESOURCE_EXHAUSTED.String(),
+ vtrpcpb.Code_FAILED_PRECONDITION.String(),
+ vtrpcpb.Code_ABORTED.String(),
+ vtrpcpb.Code_OUT_OF_RANGE.String(),
+ vtrpcpb.Code_UNIMPLEMENTED.String(),
+ vtrpcpb.Code_INTERNAL.String(),
+ vtrpcpb.Code_UNAVAILABLE.String(),
+ vtrpcpb.Code_DATA_LOSS.String(),
+ )
  // InternalErrors shows number of errors from internal components.
  InternalErrors = stats.NewCounters("InternalErrors", "Task", "StrayTransactions", "Panic", "HungQuery", "Schema", "TwopcCommit", "TwopcResurrection", "WatchdogFail")
  // Unresolved tracks unresolved items. For now it's just Prepares.

go/vt/tabletserver/tabletserver.go

@@ -1190,7 +1190,7 @@ func (tsv *TabletServer) convertError(sql string, bindVariables map[string]inter
  if strings.Contains(errstr, "read-only") {
  errCode = vtrpcpb.Code_FAILED_PRECONDITION
  }
- case 1227: // Google internal overloaded error code.
+ case 1227: // Google internal failover error code.
  if strings.Contains(errstr, "failover in progress") {
  errCode = vtrpcpb.Code_FAILED_PRECONDITION
  }
@@ -1201,7 +1201,7 @@ func (tsv *TabletServer) convertError(sql string, bindVariables map[string]inter
  case mysqlconn.ERLockWaitTimeout:
  errCode = vtrpcpb.Code_DEADLINE_EXCEEDED
  case mysqlconn.ERLockDeadlock:
- // A deadlock rollsback the transaction.
+ // A deadlock rolls back the transaction.
  errCode = vtrpcpb.Code_ABORTED
  case mysqlconn.CRServerLost:
  // Query was killed.

go/vt/tabletserver/tabletserver_test.go

@@ -357,9 +357,9 @@ func TestTabletServerAllSchemaFailure(t *testing.T) {
  err := tsv.StartService(target, dbconfigs, testUtils.newMysqld(&dbconfigs))
  defer tsv.StopService()
  // tabletsever shouldn't start if it can't access schema for any tables
- wanterr := "could not get schema for any tables"
- if err == nil || err.Error() != wanterr {
- t.Errorf("tsv.StartService: %v, want %s", err, wanterr)
+ wantErr := "could not get schema for any tables"
+ if err == nil || err.Error() != wantErr {
+ t.Errorf("tsv.StartService: %v, want %s", err, wantErr)
  }
 }
 

go/vt/tabletserver/tx_pool_test.go

@@ -177,8 +177,8 @@ func TestTxPoolBeginWithPoolConnectionError_Errno2006_Permanent(t *testing.T) {
  if !ok {
  t.Fatalf("Unexpected error type: %T, want %T", err, &sqldb.SQLError{})
  }
- if num := sqlErr.Number(); num != mysqlconn.CRServerLost {
- t.Errorf("Unexpected error code: %d, want %d", num, mysqlconn.CRServerLost)
+ if got, want := sqlErr.Number(), mysqlconn.CRServerLost; got != want {
+ t.Errorf("Unexpected error code: %d, want %d", got, want)
  }
 }
 

go/vt/vterrors/aggregate.go

@@ -25,11 +25,11 @@ const (
  PriorityOutOfRange
  // Potentially retryable errors.
  PriorityUnavailable
- PriorityFailedPrecondition
- PriorityResourceExhausted
  PriorityDeadlineExceeded
  PriorityAborted
+ PriorityFailedPrecondition
  // Permanent errors.
+ PriorityResourceExhausted
  PriorityUnknown
  PriorityUnauthenticated
  PriorityPermissionDenied

go/vt/vterrors/doc.go

@@ -18,8 +18,8 @@ RPCError message that can be used to transmit errors through RPCs, in
 the message payloads. These codes match the names and numbers defined
 by gRPC.
 
-Vitess also defines a VitessError error implementation, that can convert
-any error and add a code to it.
+Vitess also defines a standardized error implementation that allows
+you to build an error with an associated canonical code.
 
 While sending an error through gRPC, these codes are transmitted
 using gRPC's error propagation mechanism and decoded back to

go/vt/vtgate/buffer/buffer.go

@@ -214,13 +214,11 @@ func (b *Buffer) StatsUpdate(ts *discovery.TabletStats) {
 // causedByFailover returns true if "err" was supposedly caused by a failover.
 // To simplify things, we've merged the detection for different MySQL flavors
 // in one function. Supported flavors: MariaDB, MySQL, Google internal.
-// TODO(mberlin): This function does not have to check the specific error messages.
-// The previous error revamp ensures that FAILED_PRECONDITION is returned only
-// during failover.
 func causedByFailover(err error) bool {
  log.V(2).Infof("Checking error (type: %T) if it is caused by a failover. err: %v", err, err)
 
- if vterrors.Code(err) != vtrpcpb.Code_FAILED_PRECONDITION {
+ // TODO(sougou): Remove the INTERNAL check after rollout.
+ if code := vterrors.Code(err); code != vtrpcpb.Code_FAILED_PRECONDITION && code != vtrpcpb.Code_INTERNAL {
  return false
  }
  switch {

go/vt/vtgate/router_select_test.go

@@ -412,7 +412,7 @@ func TestSelectEqualFail(t *testing.T) {
  _, err := routerExec(router, "select id from user where id = (select count(*) from music)", nil)
  want := "unsupported"
  if err == nil || !strings.HasPrefix(err.Error(), want) {
- t.Errorf("routerExec: %v, must contain %v", err, want)
+ t.Errorf("routerExec: %v, must start with %v", err, want)
  }
 
  _, err = routerExec(router, "select id from user where id = :aa", nil)

go/vt/vtgate/vtgate.go

@@ -885,13 +885,13 @@ func (vtg *VTGate) VSchemaStats() *VSchemaStats {
 
 func recordAndAnnotateError(err error, statsKey []string, request map[string]interface{}, logger *logutil.ThrottledLogger) error {
  ec := vterrors.Code(err)
- fullkey := []string{
+ fullKey := []string{
  statsKey[0],
  statsKey[1],
  statsKey[2],
  ec.String(),
  }
- errorCounts.Add(fullkey, 1)
+ errorCounts.Add(fullKey, 1)
  // Most errors are not logged by vtgate beecause they're either too spammy or logged elsewhere.
  switch ec {
  case vtrpcpb.Code_UNKNOWN, vtrpcpb.Code_INTERNAL, vtrpcpb.Code_DATA_LOSS: