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

Do not retry errno 2013 in tablet #9009

Merged
merged 3 commits into from
Oct 27, 2021
Merged

Conversation

aquarapid
Copy link
Contributor

@aquarapid aquarapid commented Oct 18, 2021

Do not retry "server lost" (errno 2013) errors in vttablet.

These errors are usually due to:

  • Query getting killed on the server-side
  • Auth errors
  • Packet size exceeded errors
  • Network drops (e.g. TCP conn between tablet and MySQL server getting terminated)

Usually not helpful to retry the above.

One exception are certain network issues (have to be serious to break a TCP conn), that we may paper over with current retries. I would argue that this is not a great idea; or at least we should be more noisy about it (right now these retries only show up in metrics; users are very unlikely to notice).

One option could be to instead make this configurable, to preserve existing behavior by default. Comments welcome.

these non-retryable in the tablet. Other cases that lead to 2013 like
auth and packet size limits are unlikely to succeed on retry anyway.
One exception might be network errors, but we should we be papering
over these, anyway?

Signed-off-by: Jacques Grove <aquarapid@gmail.com>
Signed-off-by: Jacques Grove <aquarapid@gmail.com>
@aquarapid aquarapid added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) labels Oct 18, 2021
@aquarapid aquarapid requested review from mattlord and deepthi October 18, 2021 19:34
go/mysql/constants.go Outdated Show resolved Hide resolved
go/mysql/constants_test.go Outdated Show resolved Hide resolved
Signed-off-by: Jacques Grove <aquarapid@gmail.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.
@harshit-gangal can you take a look? Should we be concerned about backwards-compatibility?

@harshit-gangal
Copy link
Member

I remember we use this to re-try traffic for reserved connection cases if they gets killed due to inactivity on connection.

@harshit-gangal
Copy link
Member

This is on vttablet side, go ahead with the changes.

@aquarapid aquarapid marked this pull request as ready for review October 22, 2021 19:32
@deepthi
Copy link
Member

deepthi commented Oct 22, 2021

I remember we use this to re-try traffic for reserved connection cases if they gets killed due to inactivity on connection.

To be clear, you are talking about vtgate here, whereas the PR is for vttablet. Is that correct?

@harshit-gangal
Copy link
Member

I remember we use this to re-try traffic for reserved connection cases if they gets killed due to inactivity on connection.

To be clear, you are talking about vtgate here, whereas the PR is for vttablet. Is that correct?

Yes, not a problem here.

@deepthi deepthi merged commit 59a4447 into vitessio:main Oct 27, 2021
@deepthi deepthi deleted the jg_serverlost branch October 27, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants