[do not merge] initialize pool from scratch after TableGC.Close #164
Conversation
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes connection pool creation for TableGC, ensures pools are reinitialized after Close, and adds tests and safeguards for reopening pools and expvar registration.
- Extract pool creation into
newPoolhelper and use it in constructor,Open, andClose - Reinitialize
collector.poolif it’snilinOpen - Add tests for reopening pools and ignore duplicate expvar registrations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| go/vt/vttablet/tabletserver/gc/tablegc.go | Moved inline pool config into newPool, added reinit in Open, and clear in Close |
| go/vt/vttablet/tabletserver/connpool/pool_test.go | Added TestReopenPool & TestReopenPoolUnnamedEnv to verify reopening behavior |
| go/stats/export.go | Guard against duplicate expvar.Publish by checking expvar.Get |
Comments suppressed due to low confidence (2)
go/vt/vttablet/tabletserver/gc/tablegc.go:717
- [nitpick] The helper
newPoolname is very generic and may collide with other packages or cause confusion. Consider renaming it tonewTableGCPoolto clarify its purpose.
func newPool(env tabletenv.Env) *connpool.Pool {
go/stats/export.go:104
- There’s no test covering the case where a duplicate expvar is ignored. Adding a unit test for this guard would ensure the intended behavior is verified.
if v := expvar.Get(name); v != nil {
|
|
||
| func newPool(env tabletenv.Env) *connpool.Pool { | ||
| return connpool.NewPool(env, "TableGCPool", tabletenv.ConnPoolConfig{ | ||
| Size: 2, |
There was a problem hiding this comment.
[nitpick] The pool size 2 is hard-coded here. Extract this into a named constant or configuration so its meaning is clear and easier to adjust.
| Size: 2, | |
| Size: DefaultTableGCPoolSize, |
Copilot uses AI. Check for mistakes.
| @@ -50,6 +50,72 @@ func TestConnPoolGet(t *testing.T) { | |||
| dbConn.Recycle() | |||
| } | |||
|
|
|||
| func TestReopenPool(t *testing.T) { | |||
There was a problem hiding this comment.
[nitpick] The two tests TestReopenPool and TestReopenPoolUnnamedEnv share almost identical logic. Consider consolidating them into a table-driven test to reduce duplication.
Copilot uses AI. Check for mistakes.
| @@ -100,6 +100,11 @@ func (vg *varGroup) publish(name string, v expvar.Var) { | |||
| vg.Lock() | |||
| defer vg.Unlock() | |||
|
|
|||
| // Ignore if the variable is already registered | |||
| if v := expvar.Get(name); v != nil { | |||
There was a problem hiding this comment.
[nitpick] The inner variable v shadows the parameter v, which can be confusing. Rename the shadowed variable (e.g., existing) to improve readability.
| if v := expvar.Get(name); v != nil { | |
| if existing := expvar.Get(name); existing != nil { |
Copilot uses AI. Check for mistakes.
No description provided.