feat(chpool): add healthcheck function#1088
feat(chpool): add healthcheck function#1088nmiculinic wants to merge 2 commits intoClickHouse:mainfrom
Conversation
|
Neven Miculinic seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
nmiculinic
force-pushed
the
add_pool_healhchecking
branch
2 times, most recently
from
901e513 to
5833ddf
Compare
nmiculinic
changed the title
nmiculinic
force-pushed
the
add_pool_healhchecking
branch
from
5833ddf to
f6bed41
Compare
nmiculinic
changed the title
nmiculinic
force-pushed
the
add_pool_healhchecking
branch
2 times, most recently
from
e664181 to
c8638bb
Compare
nmiculinic
changed the title
| } | ||
| } | ||
|
|
||
| func (p *Pool) connIsHealthy(res *puddle.Resource[*connResource]) bool { |
There was a problem hiding this comment.
Happy to change this name to something better
| o.HealthCheckTimeout = DefaultHealthCheckTimeout | ||
| } | ||
| if o.ClientOptions.Logger == nil { | ||
| o.ClientOptions.Logger = zap.NewNop() |
There was a problem hiding this comment.
To prevent any nil-ptr exception later
nmiculinic
force-pushed
the
add_pool_healhchecking
branch
8 times, most recently
from
68dfbb1 to
50d4213
Compare
nmiculinic
force-pushed
the
add_pool_healhchecking
branch
from
50d4213 to
10694d0
Compare
|
@SpencerTorres @ernado mind having a look at this PR? |
|
@ernado perhaps you're more familiar with this pooling code? |
|
This code was contributed + I'm not using pools, so I'm familiar in same degree as you. |
|
@SpencerTorres @ernado any movement on this? What would make you happy to accept this PR |
|
@kavirajk Hey this one has been open for a while, would you be able to take a look? |
|
@nmiculinic apologies for the delay, could you sign the CLA? It's the first comment in this thread |
| } | ||
|
|
||
| c.res.Release() | ||
| // calling async since connIsHealthy may block |
There was a problem hiding this comment.
I'm failing to understand why move this to non-async. From the user POV, when I call c.Release() I would expect to release the connection to the pool immediately (not eventually).
| assert.Equal(t, int64(0), atomic.LoadInt64(&healthCheckCnt)) | ||
|
|
||
| conn.Release() | ||
| waitForReleaseToComplete() |
There was a problem hiding this comment.
This would just make the test more fragile IMHO. Again I understand this is the result of making the release async. See my above comment.
| c.res.Release() | ||
| // calling async since connIsHealthy may block | ||
| go func() { | ||
| if c.p.connIsHealthy(c.res) { |
There was a problem hiding this comment.
I'm also curious why is just client.IsClosed() is not good enough here? why introduce connIsHealthy based on ping() that connection?
To me it looks like your use case is not properly handled with just simple check client.IsClosed(). May I know why?
My understanding is, if the health check is passing on the given connection, it should return correct status on client.IsClosed() no? why introduce another layer of additional checks?
| res.ReleaseUnused() | ||
| } | ||
| }() | ||
| wg.Wait() |
There was a problem hiding this comment.
Also I don't think calling wg.Wait() here inside the loop will do what you expected to do. Usually you start all the goroutines (inside the for loop here). Then call wg.Wait() once all the go-routines are spawned (basically outside the loop here)
Calling inside the loop just forces each go-routine to finish before starting the another (sequential not concurrent).
May be I'm missing something here?
| assert.GreaterOrEqual(t, int64(1), atomic.LoadInt64(&healthCheckCnt)) | ||
|
|
||
| hc := atomic.LoadInt64(&healthCheckCnt) | ||
| time.Sleep(750 * time.Millisecond) |
There was a problem hiding this comment.
This random sleep going to make the tests only more fragile IMHO
5 participants
Summary
Adding chpool healthchecking option.
Defaulting to
PingChecklist
Delete items not relevant to your PR: