Use connection pool in golibmc(again)#57
Use connection pool in golibmc(again)#57windreamer merged 48 commits intodouban:masterfrom jumpeiMano:feature/connection-pool
Conversation
This reverts commit f4c6036.
|
@windreamer @ariesdevil Would you please review this PR? |
This reverts commit 3133d51.
src/golibmc.go
Outdated
| func (client *Client) putConn(cn *conn, err error) error { | ||
| client.lk.Lock() | ||
| if err == ErrBadConn || | ||
| !client.putConnLocked(cn, nil) { |
There was a problem hiding this comment.
prefer one more tab indent
| errCode := C.client_quit(cn._imp) | ||
| C.client_destroy_broadcast_result(cn._imp) | ||
| err := networkError(errorMessage[errCode]) | ||
| if isBadConnErr(err) { |
There was a problem hiding this comment.
isBadConnErr => isNetworkError ?
the meanings of "badConn" term that i used is different from ErrBadConn https://github.com/douban/libmc/pull/57/files#diff-b21c9a32b7563e26c49d7f215bc5a656R87 。
There was a problem hiding this comment.
okay, i see。 you want to put ErrBadConn together with all networkErrors，i am ok with this. ignore the above comment。
src/golibmc.go
Outdated
| if isBadConnErr(err) { | ||
| cn.client.lk.Lock() | ||
| defer cn.client.lk.Unlock() | ||
| cn.client.numOpen-- |
There was a problem hiding this comment.
"cn.client.numOpen--" may need to put out of the "if isBadConnErr(err)" block，so quit will cause "cn.client.numOpen--". how do you think?
There was a problem hiding this comment.
Oh...surely...
Thank you, fixed.
ff59967
|
|
||
| errCode := C.client_quit(client._imp) | ||
| C.client_destroy_broadcast_result(client._imp) | ||
| close(client.openerCh) |
There was a problem hiding this comment.
may close twice, check closed = true first?
| return err | ||
| } | ||
| client.lk.Unlock() | ||
| return err |
There was a problem hiding this comment.
the err returned is not used.
There was a problem hiding this comment.
Yes, putConn () is mostly called from defer. How do you think it will be better?
| return cn.createdAt.Add(timeout).Before(time.Now()) | ||
| } | ||
|
|
||
| func (cn *conn) quit() error { |
There was a problem hiding this comment.
- since quit is called internal and there is nothing todo but logging, maybe quit can log inside and not return err?
- numOpen should always -- ?
| retryTimeout C.int | ||
| lk sync.Mutex | ||
| freeConns []*conn | ||
| numOpen int |
There was a problem hiding this comment.
better expose some counters to watch the stat of the pool
There was a problem hiding this comment.
It is a difficult problem. If user update their values after I expose, the behavior of the connection pool can be crazy. For example, it seems that database/sql is not disclosed. How do you think about it?
https://github.com/golang/go/blob/master/src/database/sql/sql.go#L319-L347
src/golibmc.go
Outdated
| } | ||
| client.lk.Lock() | ||
| defer client.lk.Unlock() | ||
| if !client.putConnLocked(cn, err) { |
There was a problem hiding this comment.
client.putConnLocked(cn, err) => client.putConnLocked(cn, nil)
maybe more clear
src/golibmc.go
Outdated
| if !ok { | ||
| return nil, ErrMemcachedClosed | ||
| } | ||
| return ret.conn, nil |
There was a problem hiding this comment.
should comment at least if here assume that ret.err is nil?
There was a problem hiding this comment.
Well... I made it return ret.err.
c1060ec
src/golibmc.go
Outdated
| client.closed = true | ||
| client.lk.Unlock() | ||
| for _, cn := range client.freeConns { | ||
| if err := cn.quit(); err != nil { |
There was a problem hiding this comment.
suppose caller would always handle the err and call Quit again?
Or just go on to free them all ignoring the err (hold and return at last )?
There was a problem hiding this comment.
Thanks. I think that ignoring the quit error is better. Fixed.
aba5a7b
src/golibmc.go
Outdated
| } | ||
| client.lk.Lock() | ||
| if !client.putConnLocked(cn, nil) { | ||
| client.numOpen-- |
There was a problem hiding this comment.
https://github.com/douban/libmc/pull/57/files#diff-b21c9a32b7563e26c49d7f215bc5a656R1375
cn.quit also numOpen--，so double。
|
LGTM, thank you for very wonderful work。 |
|
cc @windreamer |
|
Ohhhhh! Thank you so much for your review! |
ChangeLog: - use pickle protocol version 2 (#52) - fix rvalue reference (#53) - Fix tests (#55) - golibmc_test: Fix fragile test case (#56) - Use connection pool in golibmc(again) (#57) - golibmc Quit ret err (#59) - cmake: Incorporate gtest in a standard way (#58) - Rename refactor (#60) - sync rapidjson/itoa.h upstream fix (#65) - split cppcheck (#67) - split FSM_GET_BYTES_CAS (#68) - Avoid noisy recv_err when broadcast quit (#73) - Try to support updating some servers without affecting others (#74) - make normalize_key cpdef & add warnings for unpickle unmarshal fail (#66) - fix fcntl usage when set socket nonblock (#69) - minor fix (#61) - add condition log macro (#77) - Define and use notWaitForRetryTimeout (#82) - upgrade travis to gcc 7 (#91) - add func errCodeToString (#79) - Introduce a reconnect mechanism in waitPoll (#88)
6 participants
#54
Since I was to close the previous PR, I made a new PR.
I'm so sorry, but please confirm this!
Thanks.