Skip to content

Commit

Permalink
update variable names, use assert.Equal whenever possible
Browse files Browse the repository at this point in the history
Signed-off-by: Serry Park <me@serry.co>
  • Loading branch information
spark4 committed Jun 22, 2020
1 parent c82a3f2 commit cc0840e
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 44 deletions.
16 changes: 8 additions & 8 deletions go/vt/sqlparser/comments.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ const (
DirectiveQueryTimeout = "QUERY_TIMEOUT_MS"
// DirectiveScatterErrorsAsWarnings enables partial success scatter select queries
DirectiveScatterErrorsAsWarnings = "SCATTER_ERRORS_AS_WARNINGS"
// DirectiveMaxPayloadSizeOverride skips payload size validation when set.
DirectiveMaxPayloadSizeOverride = "MAX_PAYLOAD_SIZE_OVERRIDE"
// DirectiveIgnoreMaxPayloadSize skips payload size validation when set.
DirectiveIgnoreMaxPayloadSize = "IGNORE_MAX_PAYLOAD_SIZE"
)

func isNonSpace(r rune) bool {
Expand Down Expand Up @@ -301,22 +301,22 @@ func SkipQueryPlanCacheDirective(stmt Statement) bool {
return false
}

// MaxPayloadSizeOverrideDirective returns true if the max payload size override
// IgnoreMaxPayloadSizeDirective returns true if the max payload size override
// directive is set to true.
func MaxPayloadSizeOverrideDirective(stmt Statement) bool {
func IgnoreMaxPayloadSizeDirective(stmt Statement) bool {
switch stmt := stmt.(type) {
case *Select:
directives := ExtractCommentDirectives(stmt.Comments)
return directives.IsSet(DirectiveMaxPayloadSizeOverride)
return directives.IsSet(DirectiveIgnoreMaxPayloadSize)
case *Insert:
directives := ExtractCommentDirectives(stmt.Comments)
return directives.IsSet(DirectiveMaxPayloadSizeOverride)
return directives.IsSet(DirectiveIgnoreMaxPayloadSize)
case *Update:
directives := ExtractCommentDirectives(stmt.Comments)
return directives.IsSet(DirectiveMaxPayloadSizeOverride)
return directives.IsSet(DirectiveIgnoreMaxPayloadSize)
case *Delete:
directives := ExtractCommentDirectives(stmt.Comments)
return directives.IsSet(DirectiveMaxPayloadSizeOverride)
return directives.IsSet(DirectiveIgnoreMaxPayloadSize)
default:
return false
}
Expand Down
18 changes: 10 additions & 8 deletions go/vt/sqlparser/comments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ limitations under the License.
package sqlparser

import (
"fmt"
"reflect"
"testing"

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

func TestSplitComments(t *testing.T) {
Expand Down Expand Up @@ -386,22 +389,21 @@ func TestSkipQueryPlanCacheDirective(t *testing.T) {
}
}

func TestMaxPayloadSizeOverrideDirective(t *testing.T) {
func TestIgnoreMaxPayloadSizeDirective(t *testing.T) {
testCases := []struct {
query string
expected bool
}{
{"insert /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ into user(id) values (1), (2)", true},
{"insert /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ into user(id) values (1), (2)", true},
{"insert into user(id) values (1), (2)", false},
{"update /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ users set name=1", true},
{"select /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ * from users", true},
{"delete /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ from users", true},
{"update /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ users set name=1", true},
{"select /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ * from users", true},
{"delete /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ from users", true},
}

for _, test := range testCases {
stmt, _ := Parse(test.query)
if got := MaxPayloadSizeOverrideDirective(stmt); got != test.expected {
t.Errorf("d.MaxPayloadSizeOverrideDirective(stmt) returned %v but expected %v", got, test.expected)
}
got := IgnoreMaxPayloadSizeDirective(stmt)
assert.Equalf(t, test.expected, got, fmt.Sprintf("d.IgnoreMaxPayloadSizeDirective(stmt) returned %v but expected %v", got, test.expected))
}
}
6 changes: 3 additions & 3 deletions go/vt/vtgate/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -1286,7 +1286,7 @@ func (e *Executor) getPlan(vcursor *vcursorImpl, sql string, comments sqlparser.
query := sql
statement := stmt
bindVarNeeds := sqlparser.BindVarNeeds{}
if !sqlparser.MaxPayloadSizeOverrideDirective(statement) && !isValidPayloadSize(query) {
if !sqlparser.IgnoreMaxPayloadSizeDirective(statement) && !isValidPayloadSize(query) {
return nil, vterrors.New(vtrpcpb.Code_RESOURCE_EXHAUSTED, "query payload size above threshold")
}

Expand Down Expand Up @@ -1500,7 +1500,7 @@ func checkLikeOpt(likeOpt string, colNames []string) (string, error) {
}

// isValidPayloadSize validates whether a query payload is above the
// configured MaxPayloadSize threshold. The PayloadSizeExceeded will increment
// configured MaxPayloadSize threshold. The WarnPayloadSizeExceeded will increment
// if the payload size exceeds the warnPayloadSize.

func isValidPayloadSize(query string) bool {
Expand All @@ -1509,7 +1509,7 @@ func isValidPayloadSize(query string) bool {
return false
}
if *warnPayloadSize > 0 && payloadSize > *warnPayloadSize {
warnings.Add("PayloadSizeExceeded", 1)
warnings.Add("WarnPayloadSizeExceeded", 1)
}
return true
}
Expand Down
36 changes: 13 additions & 23 deletions go/vt/vtgate/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1881,50 +1881,40 @@ func TestExecutorMaxPayloadSizeExceeded(t *testing.T) {

executor, _, _, _ := createExecutorEnv()
session := NewSafeSession(&vtgatepb.Session{TargetString: "@master"})
warningCount := warnings.Counts()["PayloadSizeExceeded"]
warningCount := warnings.Counts()["WarnPayloadSizeExceeded"]
testMaxPayloadSizeExceeded := []string{
"select * from main1",
"select * from main1",
"insert into main1(id) values (1), (2)",
"update main1 set id=1",
"delete from main1 where id=1",
}
for _, query := range testMaxPayloadSizeExceeded {
_, err := executor.Execute(context.Background(), "TestExecutorMaxPayloadSizeExceeded", session, query, nil)
want := "query payload size above threshold"
if err == nil || err.Error() != want {
t.Errorf("got: %v, want %s", err, want)
if err == nil {
assert.EqualError(t, err, "query payload size above threshold")
}
}
if got, want := warnings.Counts()["PayloadSizeExceeded"], warningCount; got != want {
t.Errorf("warnings count: %v, want %v", got, want)
}
assert.Equal(t, warningCount, warnings.Counts()["WarnPayloadSizeExceeded"], "warnings count")

testMaxPayloadSizeOverride := []string{
"select /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ * from main1",
"insert /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ into main1(id) values (1), (2)",
"update /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ main1 set id=1",
"delete /*vt+ MAX_PAYLOAD_SIZE_OVERRIDE=1 */ from main1 where id=1",
"select /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ * from main1",
"insert /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ into main1(id) values (1), (2)",
"update /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ main1 set id=1",
"delete /*vt+ IGNORE_MAX_PAYLOAD_SIZE=1 */ from main1 where id=1",
}
for _, query := range testMaxPayloadSizeOverride {
_, err := executor.Execute(context.Background(), "TestExecutorMaxPayloadSizeWithOverride", session, query, nil)
if err != nil {
t.Errorf("error should be nil - got: %v", err)
}
}
if got, want := warnings.Counts()["PayloadSizeExceeded"], warningCount; got != want {
t.Errorf("warnings count: %v, want %v", got, want)
assert.Equal(t, nil, err, "err should be nil")
}
assert.Equal(t, warningCount, warnings.Counts()["WarnPayloadSizeExceeded"], "warnings count")

*maxPayloadSize = 1000
for _, query := range testMaxPayloadSizeExceeded {
_, err := executor.Execute(context.Background(), "TestExecutorMaxPayloadSizeExceeded", session, query, nil)
if err != nil {
t.Errorf("error should be nil - got: %v", err)
}
}
if got, want := warnings.Counts()["PayloadSizeExceeded"], warningCount+4; got != want {
t.Errorf("warnings count: %v, want %v", got, want)
assert.Equal(t, nil, err, "err should be nil")
}
assert.Equal(t, warningCount+4, warnings.Counts()["WarnPayloadSizeExceeded"], "warnings count")
}

func TestOlapSelectDatabase(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions go/vt/vtgate/vtgate.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ var (
// HealthCheckTimeout is the timeout on the RPC call to tablets
HealthCheckTimeout = flag.Duration("healthcheck_timeout", time.Minute, "the health check timeout period")
maxPayloadSize = flag.Int("max_payload_size", 0, "The threshold for query payloads in bytes. A payload greater than this threshold will result in a failure to handle the query.")
warnPayloadSize = flag.Int("warn_payload_size", 0, "The warning threshold for query payloads in bytes. A payload greater than this threshold will cause the VtGateWarnings.PayloadSizeExceeded counter to be incremented.")
warnPayloadSize = flag.Int("warn_payload_size", 0, "The warning threshold for query payloads in bytes. A payload greater than this threshold will cause the VtGateWarnings.WarnPayloadSizeExceeded counter to be incremented.")
)

func getTxMode() vtgatepb.TransactionMode {
Expand Down Expand Up @@ -196,7 +196,7 @@ func Init(ctx context.Context, serv srvtopo.Server, cell string, tabletTypesToWa
_ = stats.NewRates("ErrorsByDbType", stats.CounterForDimension(errorCounts, "DbType"), 15, 1*time.Minute)
_ = stats.NewRates("ErrorsByCode", stats.CounterForDimension(errorCounts, "Code"), 15, 1*time.Minute)

warnings = stats.NewCountersWithSingleLabel("VtGateWarnings", "Vtgate warnings", "type", "IgnoredSet", "ResultsExceeded", "PayloadSizeExceeded")
warnings = stats.NewCountersWithSingleLabel("VtGateWarnings", "Vtgate warnings", "type", "IgnoredSet", "ResultsExceeded", "WarnPayloadSizeExceeded")

servenv.OnRun(func() {
for _, f := range RegisterVTGates {
Expand Down

0 comments on commit cc0840e

Please sign in to comment.