make sure mysql is healthy when start proxy

[nisainan]

func NewPool(...) return no error when mysql server is down, adding an error return when dialing mysql failed

close #700

[skoef]

Given that I personally don't use this connection pooling, this seems a fair change: ultimately this would return an error when a connection to the mysql backend could not be set up, ór when the handshake would fail.

[lance6716]

I don't think this is a right enhancement. Pools are usually lazy initialized, for example. sql.Open will not check if it can dail the MySQL. And users may want to implement their own checking logic, for example, start the proxy in advance, backoff and retry to see if the connection become ready.

[nisainan]

I don't think this is a right enhancement. Pools are usually lazy initialized, for example. sql.Open will not check if it can dail the MySQL. And users may want to implement their own checking logic, for example, start the proxy in advance, backoff and retry to see if the connection become ready.

Maybe I don't quite agree whit that. sql.Open() return DB object and did not do any dailing work, when I use db.Ping(),it reminds me connect error.But when I use pool.GetConn(), it will be blocked if mysql server is down. And when I do NewPool(),it has dialed mysql server ,so it has responsibility to return the result of dialing

[nisainan]

I insist on my point and thanks for your response anyway.

[lance6716]

And when I do NewPool(),it has dialed mysql server

I'm not sure about this, maybe you are refering

go-mysql/client/pool.go

Line 121 in 62e8407

go pool.newConnectionProducer()

? To me this is an asynchronous action, so when NewPool() returns we can't make sure we dailed MySQL server.

But when I use pool.GetConn(), it will be blocked if mysql server is down.

I think this is expecpted of DB.Conn, "Conn will block until either a connection is returned or ctx is canceled."

when I use db.Ping(),it reminds me connect error

You still can use it.

[nisainan]

if minAlive > 0,it will dial mysql server Immediately,check line 123

[lance6716]

if minAlive > 0,it will dial mysql server Immediately,check line 123

Sorry I didn't notice this line. Now I think this PR is reasonable, will review soon.

[lance6716]

I think we should make it clear about the definition of minAlive. As the behaviour and the comment of NewPool, "minAlive specifies the minimum number of open connections that the pool will try to maintain", "try to maintain" does not mean "must maintain", in other words does not mean it should return error when fails to maintain.

If you think the definition is "must maintain", take this as an example. After the server starts it has maintained 10 connections, now we call GetConn and for idle connection we do a ping, find that the network is bad and we can't dial new connection. "must maintain" means the whole server should be stopped at the moment.

Also since this PR will change the interface and break compatibility, I'd reject it if there's no strong reason.

[lance6716]

here we should return true? because for false caller will try to close redundant connections, but if we go here we are lack of connections.

[nisainan]

don't confuse starting and runtime,you have dailing action during starting,but you don't care the result.As you said,you just dail and it is not your business if it succeed or not.All a develop can do is using context.WithTimeout(),but can not know why failed because you did not handle the dialing error.your another opinion between try to maintain and must maintain cant stand. If I already kown mysql server is bad,why should I run this proxy successfully,unless remove dialing work during starting.That's all

[lance6716]

don't confuse starting and runtime,you have dailing action during starting,but you don't care the result.As you said,you just dail and it is not your business if it succeed or not.All a develop can do is using context.WithTimeout(),but can not know why failed because you did not handle the dialing error.your another opinion between try to maintain and must maintain cant stand. If I already kown mysql server is bad,why should I run this proxy successfully,unless remove dialing work during starting.That's all

Maybe you can add a MustNewPoolWithConns or (*Pool).MustConns so the old interface is not changed.

If other user see minAlive parameter, he/she may think it's used to reduce the warmup cost when GetConn, not a "must maintain". It's still about the definition. If you still insist to change the behaviour of NewPool, please address this case first:

If you think the definition is "must maintain", take this as an example. After the server starts it has maintained 10 connections, now we call GetConn and for idle connection we do a ping, find that the network is bad and we can't dial new connection. "must maintain" means the whole server should be stopped at the moment.

[nisainan]

Maybe it is better adding a new method than changing the behaviour of NewPool, I will fix this lately.Thanks a lot :)

[atercattus]

Hello.
Pool functionality is my code from previous work :) And the "return no error" approach was chosen specifically for proxy logic (which has been running for several years).

I'll carefully read your discussion today and will write what I think. At first glance, MustNewPoolWithConns looks like a good choice.

It's ok for you?

P.S.And sorry for so late response :)

[atercattus]

@nisainan , what do you think about this?

[atercattus]

I think NewPoolWithOptions(WithNewPoolPingTimeout()) fixes this