Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change GRPC message limit SQL error type #6630

Merged
merged 14 commits into from
Sep 17, 2020
13 changes: 12 additions & 1 deletion go/mysql/sql_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func NewSQLErrorFromError(err error) error {
case vtrpcpb.Code_UNAUTHENTICATED:
num = ERAccessDeniedError
case vtrpcpb.Code_RESOURCE_EXHAUSTED:
num = ERTooManyUserConnections
num = demuxResourceExhaustedErrors(err.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still required after changing to through mysql error directly?

Copy link
Contributor Author

@setassociative setassociative Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- if we directly map all ResourceExhausted into ERNetPacketTooLarge then we end up in the same position where we are grouping errors that don't necessarily make sense together.

Specifically when we overflow a connection pool (vttablet: rpc error: code = ResourceExhausted desc = pool ConnPool waiter count exceeded) and the like we want to distinguish that as something that can be solved via backoff (ResourceExhausted) instead of queries that are inherently less retryable because they are likely to generate "bad" data (ErNetPacketTooLarge).

case vtrpcpb.Code_FAILED_PRECONDITION:
num = ERUnknownError
case vtrpcpb.Code_ABORTED:
Expand Down Expand Up @@ -161,3 +161,14 @@ func NewSQLErrorFromError(err error) error {
}
return serr
}

var isGRPCOverflowRE = regexp.MustCompile(`.*grpc: received message larger than max \(\d+ vs. \d+\)`)

func demuxResourceExhaustedErrors(msg string) int {
setassociative marked this conversation as resolved.
Show resolved Hide resolved
switch {
case isGRPCOverflowRE.Match([]byte(msg)):
return ERNetPacketTooLarge
default:
return ERTooManyUserConnections
}
}
45 changes: 45 additions & 0 deletions go/mysql/sql_error_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
Copyright 2020 The Vitess Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package mysql

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestDumuxResourceExhaustedErrors(t *testing.T) {
type testCase struct {
msg string
want int
}

cases := []testCase{
testCase{"misc", ERTooManyUserConnections},
testCase{"grpc: received message larger than max (99282 vs. 1234): trailer", ERNetPacketTooLarge},
testCase{"grpc: received message larger than max (1234 vs. 1234)", ERNetPacketTooLarge},
testCase{"header: grpc: received message larger than max (1234 vs. 1234)", ERNetPacketTooLarge},
// This should be explicitly handled by returning ERNetPacketTooLarge from the execturo directly
// and therefore shouldn't need to be teased out of another error.
testCase{"in-memory row count exceeded allowed limit of 13", ERTooManyUserConnections},
}

for _, c := range cases {
got := demuxResourceExhaustedErrors(c.msg)
assert.Equalf(t, c.want, got, c.msg)
}
}
2 changes: 1 addition & 1 deletion go/vt/vtgate/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,7 @@ func (e *Executor) getPlan(vcursor *vcursorImpl, sql string, comments sqlparser.
statement := stmt
bindVarNeeds := sqlparser.BindVarNeeds{}
if !sqlparser.IgnoreMaxPayloadSizeDirective(statement) && !isValidPayloadSize(query) {
return nil, vterrors.New(vtrpcpb.Code_RESOURCE_EXHAUSTED, "query payload size above threshold")
return nil, mysql.NewSQLError(mysql.ERNetPacketTooLarge, "", "query payload size above threshold")
}
ignoreMaxMemoryRows := sqlparser.IgnoreMaxMaxMemoryRowsDirective(stmt)
vcursor.SetIgnoreMaxMemoryRows(ignoreMaxMemoryRows)
Expand Down
4 changes: 2 additions & 2 deletions go/vt/vtgate/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func TestExecutorMaxMemoryRowsExceeded(t *testing.T) {
err string
}{
{"select /*vt+ IGNORE_MAX_MEMORY_ROWS=1 */ * from main1", ""},
{"select * from main1", "in-memory row count exceeded allowed limit of 3"},
{"select * from main1", "in-memory row count exceeded allowed limit of 3 (errno 1153) (sqlstate HY000)"},
}

for _, test := range testCases {
Expand Down Expand Up @@ -1956,7 +1956,7 @@ func TestExecutorMaxPayloadSizeExceeded(t *testing.T) {
for _, query := range testMaxPayloadSizeExceeded {
_, err := executor.Execute(context.Background(), "TestExecutorMaxPayloadSizeExceeded", session, query, nil)
require.NotNil(t, err)
assert.EqualError(t, err, "query payload size above threshold")
assert.EqualError(t, err, "query payload size above threshold (errno 1153) (sqlstate HY000)")
}
assert.Equal(t, warningCount, warnings.Counts()["WarnPayloadSizeExceeded"], "warnings count")

Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtgate/legacy_scatter_conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ func TestMaxMemoryRows(t *testing.T) {
err string
}{
{true, ""},
{false, "in-memory row count exceeded allowed limit of 3"},
{false, "in-memory row count exceeded allowed limit of 3 (errno 1153) (sqlstate HY000)"},
}

for _, test := range testCases {
Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtgate/scatter_conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ func (stc *ScatterConn) ExecuteMultiShard(
)

if !ignoreMaxMemoryRows && len(qr.Rows) > *maxMemoryRows {
return nil, []error{vterrors.Errorf(vtrpcpb.Code_RESOURCE_EXHAUSTED, "in-memory row count exceeded allowed limit of %d", *maxMemoryRows)}
return nil, []error{mysql.NewSQLError(mysql.ERNetPacketTooLarge, "", "in-memory row count exceeded allowed limit of %d", *maxMemoryRows)}
}

return qr, allErrors.GetErrors()
Expand Down