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

mysql: Add ConnectionReady callback to Handler interface #9496

Merged
merged 1 commit into from
Jan 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions go/mysql/conn_flaky_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,6 +820,10 @@ func (t testRun) NewConnection(c *Conn) {
panic("implement me")
}

func (t testRun) ConnectionReady(c *Conn) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just filling in all of the other internal implementations of the Handler interface.

When doing this, I was thinking it might be nice to introduce a stub UnimplementedHandler struct that all implementation can embed that provides default no-op handlers. Similar to how gRPC gives you the stub to satisfy the interface. That way an implementation isn't require to implement every callback if they're not needed.

I'd be happy to follow up with an UnimplementedHandler struct if you also think this is valuable. It'd help wiht the boilerplate in the tests and all that get touched in this PR.

panic("implement me")
}

func (t testRun) ConnectionClosed(c *Conn) {
panic("implement me")
}
Expand Down
5 changes: 5 additions & 0 deletions go/mysql/fakesqldb/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,11 @@ func (db *DB) NewConnection(c *mysql.Conn) {
db.connections[c.ConnectionID] = c
}

// ConnectionReady is part of the mysql.Handler interface.
func (db *DB) ConnectionReady(c *mysql.Conn) {

}

// ConnectionClosed is part of the mysql.Handler interface.
func (db *DB) ConnectionClosed(c *mysql.Conn) {
db.mu.Lock()
Expand Down
6 changes: 6 additions & 0 deletions go/mysql/mysql_fuzzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ type fuzztestRun struct{}
func (t fuzztestRun) NewConnection(c *Conn) {
}

func (t fuzztestRun) ConnectionReady(c *Conn) {
}

func (t fuzztestRun) ConnectionClosed(c *Conn) {
}

Expand Down Expand Up @@ -275,6 +278,9 @@ func (th *fuzzTestHandler) NewConnection(c *Conn) {
th.lastConn = c
}

func (th *fuzzTestHandler) ConnectionReady(_ *Conn) {
}

func (th *fuzzTestHandler) ConnectionClosed(_ *Conn) {
}

Expand Down
8 changes: 8 additions & 0 deletions go/mysql/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ type Handler interface {
// In particular, ServerStatusAutocommit might be set.
NewConnection(c *Conn)

// ConnectionReady is called after the connection handshake, but
// before we begin to process commands.
ConnectionReady(c *Conn)

// ConnectionClosed is called when a connection is closed.
ConnectionClosed(c *Conn)

Expand Down Expand Up @@ -470,6 +474,10 @@ func (l *Listener) handle(conn net.Conn, connectionID uint32, acceptTime time.Ti
log.Warningf("Slow connection from %s: %v", c, connectTime)
}

// Tell our handler that we're finished handshake and are ready to
// process commands.
l.handler.ConnectionReady(c)
Comment on lines +477 to +479
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to where we think it's best to determine Ready. It seemed to me to put it right before the for{} loop that started reading commands. But wasn't sure if yall had an opinion on putting it before the SlowConnectionWarnThreshold check, since in theory this callback can add extra connection time. I at least felt it was important to put after the last OK packet sent.


for {
kontinue := c.handleNextCommand(l.handler)
if !kontinue {
Expand Down
3 changes: 3 additions & 0 deletions go/mysql/server_flaky_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ func (th *testHandler) NewConnection(c *Conn) {
th.lastConn = c
}

func (th *testHandler) ConnectionReady(_ *Conn) {
}

func (th *testHandler) ConnectionClosed(_ *Conn) {
}

Expand Down
2 changes: 2 additions & 0 deletions go/vt/vtgate/plugin_mysql_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ func (vh *vtgateHandler) NewConnection(c *mysql.Conn) {
vh.connections[c] = true
}

func (vh *vtgateHandler) ConnectionReady(_ *mysql.Conn) {}

func (vh *vtgateHandler) numConnections() int {
vh.mu.Lock()
defer vh.mu.Unlock()
Expand Down
3 changes: 3 additions & 0 deletions go/vt/vtgate/plugin_mysql_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ func (th *testHandler) NewConnection(c *mysql.Conn) {
th.lastConn = c
}

func (th *testHandler) ConnectionReady(c *mysql.Conn) {
}

func (th *testHandler) ConnectionClosed(c *mysql.Conn) {
}

Expand Down