Replace the pool when reopening. #183
Conversation
This is a backport of vitessio#18530
There was a problem hiding this comment.
Pull Request Overview
This PR implements pool replacement functionality when reopening connections by refactoring the statistics registration system and adding pool state management. The change replaces direct stats registration with a new StatsExporter pattern that maintains atomic references to pools, enabling safe pool replacement after closure.
- Replaces direct
RegisterStatscalls with a newStatsExportersystem that uses atomic pointers - Adds pool state management (UNINITIALIZED, OPENED, CLOSED) to track connection pool lifecycle
- Implements pool replacement in
Close()methods by creating new pool instances
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| go/vt/vttablet/tabletserver/connpool/pool.go | Updates Pool struct to store config and use StatsExporter, replaces pool on close |
| go/vt/dbconnpool/connection_pool.go | Similar updates to ConnectionPool with config storage and pool replacement |
| go/pools/smartconnpool/pool_test.go | Simplifies test by removing goroutine and timeout handling |
| go/pools/smartconnpool/pool.go | Adds state management, new StatsExporter class, and improved close handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
|
||
| cp.statsExporter = smartconnpool.NewStatsExporter[*DBConnection](stats, name) | ||
|
|
||
| // cp.ConnPool.RegisterStats(stats, name) |
There was a problem hiding this comment.
Remove this commented-out code since it's been replaced by the StatsExporter pattern and is no longer needed.
| // cp.ConnPool.RegisterStats(stats, name) |
|
|
||
| if pool.active.Load() == 0 { | ||
| close(pool.close) | ||
| } |
There was a problem hiding this comment.
There's a potential race condition here. The active count could change between the Load() call and the close() call. Consider using a mutex or atomic operation to ensure thread safety when checking active connections and closing the channel.
| if pool.IsClosed() { | ||
| conn = pool.pop(&pool.settings[stack]) | ||
| conn.Close() | ||
| pool.closedConn() |
There was a problem hiding this comment.
This code pushes a connection to the clean stack and immediately pops it back. This is inefficient and could cause issues if other goroutines are accessing the stack. Consider closing the connection directly without adding it to the stack first.
| pool.closedConn() | |
| // Close connection if pool is closed | |
| if pool.IsClosed() { | |
| conn.Close() | |
| pool.closedConn() | |
| } else { | |
| pool.clean.Push(conn) | |
| } | |
| } else { | |
| stack := connSetting.bucket & stackMask | |
| // Close connection if pool is closed | |
| if pool.IsClosed() { | |
| conn.Close() | |
| pool.closedConn() | |
| } else { | |
| pool.settings[stack].Push(conn) | |
| pool.freshSettingsStack.Store(int64(stack)) |
| conn = pool.pop(&pool.settings[stack]) | ||
| conn.Close() | ||
| pool.closedConn() | ||
| } |
There was a problem hiding this comment.
Similar to the previous issue, this code pushes a connection to the settings stack and immediately pops it back. This creates unnecessary stack operations and potential race conditions. The connection should be closed directly without stack manipulation.
| } | |
| if pool.IsClosed() { | |
| conn.Close() | |
| pool.closedConn() | |
| return false | |
| } | |
| if connSetting == nil { | |
| pool.clean.Push(conn) | |
| } else { | |
| stack := connSetting.bucket & stackMask | |
| pool.settings[stack].Push(conn) | |
| pool.freshSettingsStack.Store(int64(stack)) |
This is a backport of vitessio#18530
Description
Related Issue(s)
Checklist
Deployment Notes