Skip to content

Comments

vtgateproxy v19 buildable and passing tests#594

Merged
dedelala merged 7 commits intovtgateproxy-v19from
vtgateproxy-v19-buildable
Feb 3, 2025
Merged

vtgateproxy v19 buildable and passing tests#594
dedelala merged 7 commits intovtgateproxy-v19from
vtgateproxy-v19-buildable

Conversation

@dedelala
Copy link

Description

  • vtgateproxy-v19 branch was created by cherry-picking the commits from vtgateproxy-15 onto the end of slack-19.0
  • this pr has changes required to get the vtgateproxy binary to build and pass e2e tests

changes from 15 to 19 were more substantial than 14 to 15

  • changes to mysql_server.go bring it into line with the mysql.Handler interface
    • mostly non-functional
    • adding stub methods
    • reordering slightly to the same order as the interface definition
    • added a couple of new tunables used by mysql.NewListener as flags with default values
  • handle error return from sqlescape.UnescapeID
  • update go-mysql-driver used in e2e tests (for some reason connection attributes stopped working)
  • return ErrNoSubConnAvailable instead of "no available connections" when first_ready builder is called with an empty map, this causes grpc to retry until a conn becomes ready. Otherwise fail fast logic is causing us to fail a bit too fast, something changed in this behaviour from grpc 1.45 (v15) to 1.62 (v19)

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
@dedelala dedelala requested a review from a team as a code owner January 30, 2025 03:04
@github-actions github-actions bot added this to the v19.0.7 milestone Jan 30, 2025
Copy link

@henryr henryr left a comment

Choose a reason for hiding this comment

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

lgtm!

if strings.HasPrefix(sql, "use ") {
targetString := sqlescape.UnescapeID(sql[4:])
targetString, err := sqlescape.UnescapeID(sql[4:])
if err != nil {
Copy link

Choose a reason for hiding this comment

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

Somewhat minor question but actually a bit thought provoking as well...

Suppose this parsing fails for whatever reason. Is it better to fail (as you did here) or just fall through and call session.Execute below? Since the whole point of this use statement hijacking is to avoid a round trip, if for some rare reason vtgate is able to process it but the proxy isn't then could we be unnecesarily failing?

Then again maybe the case where we can't unescape an ID is rare enough that it's just plain irrelevant?

@henryr curious about your thoughts as well.

Copy link

Choose a reason for hiding this comment

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

I think it's ok to fail. I think it's just as likely that we create a dependency elsewhere in the proxy where we assume that we've always successfully intercepted use statements, as a situation arises where we can't unescape the argument.

@dedelala dedelala merged commit 94c7f03 into vtgateproxy-v19 Feb 3, 2025
158 of 166 checks passed
@dedelala dedelala deleted the vtgateproxy-v19-buildable branch February 3, 2025 03:41
dedelala added a commit that referenced this pull request Feb 7, 2025
* get v19 to build (WIP)

* sort of builds?

* misnamed flag

* flag name

* remove binary

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* update go-mysql-driver

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* return ErrNoSubConnAvailable for no available connections

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

---------

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
dedelala added a commit that referenced this pull request Apr 3, 2025
* get v19 to build (WIP)

* sort of builds?

* misnamed flag

* flag name

* remove binary

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* update go-mysql-driver

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* return ErrNoSubConnAvailable for no available connections

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

---------

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
dedelala added a commit that referenced this pull request Apr 3, 2025
* get v19 to build (WIP)

* sort of builds?

* misnamed flag

* flag name

* remove binary

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* update go-mysql-driver

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* return ErrNoSubConnAvailable for no available connections

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

---------

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
dedelala added a commit that referenced this pull request Apr 3, 2025
* get v19 to build (WIP)

* sort of builds?

* misnamed flag

* flag name

* remove binary

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* update go-mysql-driver

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

* return ErrNoSubConnAvailable for no available connections

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>

---------

Signed-off-by: Esme Lamb <dlamb@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants