-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Added a method to create a mysql database from a connection object #583
Conversation
Pull Request Test Coverage Report for Build 492
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
database/mysql/mysql.go
Outdated
if err := mx.ensureVersionTable(); err != nil { | ||
return nil, err | ||
} | ||
|
||
return mx, nil | ||
} | ||
|
||
func (m *Mysql) setupDefaultConfig() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move all of this logic to WithConnection()
. Then inside WithInstance()
we can call WithConnection()
and set m.db = instance
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I considered both designs and I seem to have picked the wrong one 😭
I'll change this ASAP, thanks for your review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
By the way, I just reminded why I chose the previous pattern: It's because now I need WithConnection
to return a *Mysql
type instead of database.Driver
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a *Mysql
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because WithInstance
needs to set mx.db
This is fine. We can add |
1b8c309
to
394279a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback!
|
||
// instance must have `multiStatements` set to true | ||
func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { | ||
if err := instance.Ping(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to WithConnection()
and use conn.PingContext()
. DB.Ping()
will create a connection to ping anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I am trying to solve is that this class does unexpected stuff to my connection pool.
If I inject an active pool with n
idle connections to this class, do my stuff and close it, I expect my pool to be returned to it's initial state, which means an active pool with n
idle connections.
But currently I can only choose between ending-up with a closed pool (because Close
not only returns the connection to the pool, but also closes the pool itself), or (if I don't call Close
) a pool with n-1
idle connections and 1
allocated connection that will be stuck forever
So the last thing I want is for WithConnection
to create new connections in a pool that I did not even inject in the first place.
If the connection I inject is not working, I expect the process to fail without side-effects on my pool.
If the connection is not working, doing conn.PingContext().DB.Ping()
would create a new connection in the pool and ping the new connection, which does not test or fix the injected one anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that connections are pooled at the sql.DB
instance level so conn.PingContext()
shouldn't create a new connection e.g. connections won't create new connections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was misunderstanding your question.
I added the context to WithConnection
, as well as calling PingContext
.
I cannot add it as a parameter in WithInstance
without breaking change, so it should be done along with #14.
database/mysql/mysql.go
Outdated
// instance must have `multiStatements` set to true | ||
func WithInstance(instance *sql.DB, config *Config) (database.Driver, error) { | ||
// connection instance must have `multiStatements` set to true | ||
func WithConnection(conn *sql.Conn, config *Config) (*Mysql, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on making this context aware? Eventually, we want most DB driver methods to be context aware. I'm thinking that new methods should be context aware.
Also, unfortunately, these functions that create DB drivers currently return interfaces instead of concrete times. I'm thinking returning a concrete type as you're doing now is a cleaner pattern. Can you add a type check? e.g. var _ database.Driver = (*Mysql)(nil) // explicit compile time type check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's consistent across all drivers, then it would be a good thing. But it seems out of scope for this PR.
Can you add a type check?
Sorry, I don't understand what you are asking. Where do you want this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's consistent across all drivers, then it would be a good thing. But it seems out of scope for this PR.
Oh yeah, making the db drivers consistent is not in the scope of the PR. But new code should fit within the future plans/vision if possible. In this case, we should add a ctx context.Context
parameter to WithConnection()
Can you add a type check?
Sorry, I don't understand what you are asking. Where do you want this check?
Add var _ database.Driver = (*Mysql)(nil) // explicit compile time type check
as a package level ignored variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I understood your point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (both of it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR and for addressing my feedback!
@dhui Thanks for merging my PR 🎉 |
Release are blocked, so use the commit hash |
|
This PR implements #578 by adding the
WithConnection
method to the MySQL driver.I could test and verify that it works. However, I don't have the environment or time to provide and test the same change on all other drivers, so it creates a small inconsistency. How do you want to proceed?