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

Implement Connector and DriverContext interface #778

Closed

Conversation

shogo82148
Copy link
Contributor

@shogo82148 shogo82148 commented Apr 7, 2018

Description

Implemented the Connector interface and DriverContext interface in go 1.10.
fixes #671

#705 also implements Connector interface, while the context support is incomplete.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@nemith
Copy link
Contributor

nemith commented Apr 20, 2018

See #705

@shogo82148
Copy link
Contributor Author

I did. but #705 is missing some features.

  • MySQLDriver doesn't implement driver.DriverContext.
  • It is not able to cancel the context during handshaking.

@julienschmidt julienschmidt added this to the v1.5.0 milestone May 21, 2018
Copy link
Contributor

@dolmen dolmen left a comment

Choose a reason for hiding this comment

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

  • To simplify the API, the Connector should be merged with Config: just implement the Connect and Driver methods directly on *Config. OpenConnector would become a one-line wrapper around ParseDSN.

  • Rename connector.go to connector_go1.10.go, driver_go110.go to driver_go1.10.go, driver_go110_test.go to driver_go1.10_test.go because the Go toolchain has special support for this naming scheme. This makes the // +build go1.10 lines redundant, so remove them. (see also Rename go version dependent sources using idiomatic naming #822 that applies this to all existing files)

  • You duplicated the content of Driver.Open method in Connect. This open the way to maintainance nightmare (bug could be fixed in one place but forgotten in the other). A refactoring is necessary to remove this duplication. I propose to move the common bits as a private method of Config.

@dolmen
Copy link
Contributor

dolmen commented Jun 15, 2018

Do not merge master into your own branch. Instead, use rebase!

@methane
Copy link
Member

methane commented Jun 15, 2018

Do not merge master into your own branch. Instead, use rebase!

We use "squash and merge". So no need to rebase.
I prefer "merge master" over rebase, because it makes clear what changed from last review.

@dolmen dolmen mentioned this pull request Jun 15, 2018
5 tasks
@@ -0,0 +1,17 @@
// Go MySQL Driver - A MySQL-Driver for Go's database/sql package
//
// Copyright 2013 The Go-MySQL-Driver Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

For new code, use current year.

because the Go toolchain has special support for this naming scheme.
@shogo82148
Copy link
Contributor Author

Will we? #822 (comment)
If we drop Go 1.7, I can rewrite MySQLDriver.Open the following.

// Open new Connection.
// See https://github.com/go-sql-driver/mysql#dsn-data-source-name for how
// the DSN string is formated
func (d MySQLDriver) Open(dsn string) (driver.Conn, error) {
	c, err := ParseDSN(dsn)
	if err != nil {
		return nil, err
	}
	return c.Connect(context.Background())
}

This is more simple solution for duplicated code.

@shogo82148
Copy link
Contributor Author

The context package has been supported from Go 1.6, and Go 1.6 has been dropped.
So I can rewrite MySQLDriver.Open as #778 (comment), and I did it. ee66ae6

connector.go Outdated
maxAllowedPacket: maxPacketSize,
maxWriteSize: maxPacketSize - 1,
closech: make(chan struct{}),
cfg: c,
Copy link

Choose a reason for hiding this comment

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

Before this change, a connection would get its own *Config from ParseDSN. Here, each connection returned by Connect shares a pointer to the one Config. Is that intentional/desirable?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally all connections would share one immutable copy.
One goal of the connector interface was to avoid having to parse and allocate a new config for each connection.

connector.go Outdated
// Call startWatcher for context support (From Go 1.8)
if s, ok := interface{}(mc).(watcher); ok {
s.startWatcher()
if err := s.watchCancel(ctx); err != nil {
Copy link

Choose a reason for hiding this comment

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

This looks new during Connect. What does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It cancels opening new connection, if the context is canceled.
See #608 please.

Other methods which supports the context have this lines. (e.g.

mysql/connection_go18.go

Lines 84 to 87 in 2307b45

if err := mc.watchCancel(ctx); err != nil {
return nil, err
}
defer mc.finish()
)
I did not need it because MySQLDriver.Open did not correspond to the context so far, but since Config.Connect become to correspond to the context, I added this

@cbandy
Copy link

cbandy commented Aug 23, 2018

Is there anything I can do to help this along?

@cbandy
Copy link

cbandy commented Oct 26, 2018

The conflict appears to be this small change in driver.go: 7ac0064#diff-749da71b40f8ff06fc9e78ce917b0cce

@shogo82148
Copy link
Contributor Author

I resolved the conflict. Check it, please.

@methane
Copy link
Member

methane commented Oct 29, 2018

I don't like Config implements Connector interface.

type Connector struct {
    cfg *Config  // immutable private copy.
}

func NewConnector(cfg *Config) *Connector {
  copyCfg := *cfg
  // deep copy some members
  return &Connector{cfg: cfg}
}

func (c *Connector) Connect(ctx context.Context) (driver.Conn, error) {
    // ...
}

This allows future optimization for us. For example, we can use one dedicated killing connection,
instead of connect when cancelling.

@shogo82148
Copy link
Contributor Author

It sounds good.
I will add the Connector struct.

@nemith
Copy link
Contributor

nemith commented Jan 22, 2019

Since this is the real solution to ":" in username any chance we can get this into the driver?

@vmg vmg mentioned this pull request Mar 29, 2019
5 tasks
@methane methane closed this Apr 4, 2019
@shogo82148 shogo82148 deleted the connector-interface-support branch August 18, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Connector interface
6 participants