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.

Sign up for GitHub

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

Jump to bottom

Change Error Messages In VTGate #7514

Merged
merged 27 commits into from
Mar 2, 2021
Merged

Change Error Messages In VTGate #7514

merged 27 commits into from
Mar 2, 2021

Conversation

harshit-gangal
Copy link
Member

@harshit-gangal harshit-gangal commented Feb 18, 2021 •
edited
Loading

Signed-off-by: Harshit Gangal [email protected]

Description

Related Issue(s)

#7533

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving

@deepthi deepthi mentioned this pull request Feb 22, 2021
Merged
8 tasks
@harshit-gangal
added state to vterrors
9676479 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
added data out of range error state
1e00148 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
handle no db and wrong column number error
62922a4 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
return 1815 for vitess internal error
ef0b276 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
moved vterrors state to sql error conversion to mysql package
5b46e1d 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
assigned correct sql state to the vterrors codes
fe18783 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
added badfielderror
931ddc6 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
create and drop db mysql error
df06b99 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
renamed 42000 sql state to client error
d608454 
Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
added forbid schema, packet too large and savepoint no exists error t…
7c3de7d 
…o vterrors state

Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal
removed usage of generating mysql error inside vtgate code, instead g…
dfbf58d 
…enerate vterrors errors that will be converted later to mysql errors

Signed-off-by: Harshit Gangal <[email protected]>
@harshit-gangal harshit-gangal marked this pull request as ready for review February 26, 2021 20:18
systay
systay reviewed Mar 1, 2021
View reviewed changes
go/mysql/sql_error.go
@@ -162,6 +149,56 @@ func NewSQLErrorFromError(err error) error {
return serr
}

var stateToMysqlCode = map[vterrors.State]struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

systay
systay reviewed Mar 1, 2021
View reviewed changes
go/vt/vterrors/vterrors.go
@@ -185,6 +188,24 @@ func Code(err error) vtrpcpb.Code {
return vtrpcpb.Code_UNKNOWN
}

// ErrState returns the error state if it's a vtError.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

systay
systay reviewed Mar 1, 2021
View reviewed changes
go/vt/vtgate/engine/concatenate.go
}
for i, field2 := range fields2 {
field1 := fields1[i]
if field1.Type != field2.Type {
return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "column field type does not match for name: (%v, %v) types: (%v, %v)", field1.Name, field2.Name, field1.Type, field2.Type)
return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "merging field of different types is not supported, name: (%v, %v) types: (%v, %v)", field1.Name, field2.Name, field1.Type, field2.Type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@harshit-gangal
added license
1221285 
Signed-off-by: Harshit Gangal <[email protected]>
systay
systay reviewed Mar 1, 2021
View reviewed changes
go/vt/vtgate/vcursor_impl.go Outdated
if !allowed {
return vterrors.Errorf(vtrpcpb.Code_PERMISSION_DENIED, "not authorized to perform vschema operations")

return vterrors.NewErrorf(vtrpcpb.Code_PERMISSION_DENIED, vterrors.AccessDeniedError, "User '%s' to perform vschema operations", user.GetUsername())
Copy link
Collaborator

@systay systay Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this message looks a bit weird to me.
shouldn't it say: User '%s' is not authorized to perform vschema operations

@systay
remove vtgate URL from errors
08765a7 
Signed-off-by: Andres Taylor <[email protected]>
@systay systay force-pushed the fix-vtgate-errors branch from dd7e031 to 08765a7 Compare March 1, 2021 09:27
@systay systay force-pushed the fix-vtgate-errors branch from 619cb1d to 2ab202b Compare March 1, 2021 11:34
systay
systay approved these changes Mar 1, 2021
View reviewed changes
Copy link
Collaborator

@systay systay left a comment

Choose a reason for hiding this comment

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

Really nice work. 👏
I was thinking we'd let @deepthi look at the changes in 2ab202b to make sure they look ok.

@systay systay requested a review from deepthi March 1, 2021 11:36
@deepthi
Copy link
Member

deepthi commented Mar 1, 2021

Really nice work.
I was thinking we'd let @deepthi look at the changes in 2ab202b to make sure they look ok.

I think the change means it may get harder to debug/diagnose certain failures. How about logging err when it is not-nil? Do you think that might be useful?

@harshit-gangal
Copy link
Member Author

Really nice work.
I was thinking we'd let @deepthi look at the changes in 2ab202b to make sure they look ok.

I think the change means it may get harder to debug/diagnose certain failures. How about logging err when it is not-nil? Do you think that might be useful?

The error is always send back to client. If the target is not nil then it is added to the error message.
The change is that we removed the tablet url to be added to the error message.

I will go ahead and merge the PR. If there is any concern we can fix forward.

@harshit-gangal harshit-gangal merged commit 451279d into vitessio:master Mar 2, 2021
@harshit-gangal harshit-gangal deleted the fix-vtgate-errors branch March 2, 2021 05:56
@askdba askdba added this to the v10.0 milestone Mar 4, 2021
@ajm188 ajm188 mentioned this pull request Jul 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@systay systay systay approved these changes

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach

@sougou sougou Awaiting requested review from sougou

@deepthi deepthi Awaiting requested review from deepthi

Assignees

@systay systay

Labels
Component: Query Serving
Projects
None yet
Milestone
v10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@harshit-gangal @deepthi @systay @askdba