mysql: Ensure we set up the initial collation correctly #15115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbussink
requested review from
harshit-gangal,
systay,
frouioui,
GuptaManan100,
mattlord and
deepthi
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
dbussink
removed
NeedsDescriptionUpdate
GrahamCampbell
approved these changes
Feb 1, 2024
ajm188
approved these changes
Feb 1, 2024
| connectionID: 1, | ||
| connReadTimeout: cfg.ConnReadTimeout, | ||
| connWriteTimeout: cfg.ConnWriteTimeout, | ||
| connReadBufferSize: cfg.ConnReadBufferSize, | ||
| connBufferPooling: cfg.ConnBufferPooling, | ||
| connKeepAlivePeriod: cfg.ConnKeepAlivePeriod, | ||
| flushDelay: cfg.FlushDelay, | ||
| truncateErrLen: cfg.TruncateErrLen, | ||
| truncateErrLen: cfg.Handler.Env().TruncateErrLen(), | ||
| charset: cfg.Handler.Env().CollationEnv().DefaultConnectionCharset(), |
mattlord
approved these changes
Feb 2, 2024
| @@ -315,15 +306,16 @@ func NewListenerWithConfig(cfg ListenerConfig) (*Listener, error) { | |||
| authServer: cfg.AuthServer, | |||
| handler: cfg.Handler, | |||
| listener: l, | |||
| ServerVersion: cfg.MySQLServerVersion, | |||
| ServerVersion: cfg.Handler.Env().MySQLVersion(), | |||
There was a problem hiding this comment.
Can Handler and/or Env() be nil here? IMO it's worth a safety check here that returns an error or even an explicit panic with a message if it really shouldn't happen and we can't proceed. Same for the cfg.Handler chains below.
There was a problem hiding this comment.
It should never ever be nil under any circumstance. That it panics in that case is good. I don't think we should guard this, as it means many guards all over the place then (the env is used in many other places too).
The collation env refactor missed a bunch of setup for the MySQL listener, which resulted in a missing initial collation on the connection. This fixes that and adds additional assertions as well. Signed-off-by: Dirkjan Bussink <[email protected]>
Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
force-pushed
the
fix-connection-charset
branch
from
6a8f4d3 to
08408dc
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15115 +/- ##
=======================================
Coverage 70.63% 70.63%
=======================================
Files 1375 1375
Lines 182222 182217 -5
=======================================
+ Hits 128705 128708 +3
+ Misses 53517 53509 -8 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The collation env refactor missed a bunch of setup for the MySQL listener, which resulted in a missing initial collation on the connection.
This fixes that and adds additional assertions as well.
Related Issue(s)
Fixes #15112
Checklist