Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #623 +/- ##
==========================================
+ Coverage 88.12% 88.19% +0.06%
==========================================
Files 202 203 +1
Lines 21328 21644 +316
==========================================
+ Hits 18796 19089 +293
- Misses 2532 2555 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aral01
changed the title
341aecc to
9f667d2
Compare
9f667d2 to
5e45ca9
Compare
| /// Shutdown must run while the Tokio runtime that created the driver is still alive. | ||
| #[instrument(name = "ydb.Driver.Shutdown", skip_all, fields(db.system.name = "ydb", db.namespace = %self.credentials.database), err)] | ||
| pub async fn shutdown(self) -> YdbResult<()> { | ||
| self.session_pool.shutdown().await |
There was a problem hiding this comment.
[P2] This only shuts down the currently configured pool. with_session_pool replaces self.session_pool without shutting down or retaining the previous pool, while query/table clients created before replacement keep clones of that old pool. Consequently, after reconfiguration this method can return while old leases and cleanup tasks are still active, and those older clients can still acquire sessions. That defeats the documented shutdown guarantee and can still lose cleanup when the runtime is dropped. Please include superseded pools in the shutdown lifecycle or shut down the old pool during replacement.
— 🤖 AI review on behalf of @rekby
There was a problem hiding this comment.
This still applies at the current head: with_session_pool replaces the field via Self { session_pool, ..self }, while shutdown only awaits self.session_pool.shutdown(). A previously created QueryClient or TableClient therefore retains an untracked clone of the superseded pool and can keep acquiring sessions after this shutdown returns.
— 🤖 AI review on behalf of @rekby
There was a problem hiding this comment.
Still unresolved at f60ecbf: the new commit only bounds cleanup connection acquisition; with_session_pool still replaces the pool, and shutdown still awaits only the replacement. Previously derived clients can therefore retain and use the superseded pool.
— 🤖 AI review on behalf of @rekby
There was a problem hiding this comment.
Still unresolved at 97be54b3: with_session_pool still replaces the field with Self { session_pool, ..self }, while shutdown awaits only that replacement. A query/table client created before replacement therefore retains the superseded pool and can continue acquiring sessions after this shutdown completes.
— 🤖 AI review on behalf of @rekby
There was a problem hiding this comment.
Still unresolved at ac216fe: this push only changes cleanup timeout coverage. with_session_pool still replaces the pool via Self { session_pool, ..self }, while shutdown awaits only self.session_pool. Query/table clients created before replacement therefore retain the original pool and can still acquire sessions after shutdown returns.
— 🤖 AI review on behalf of @rekby
There was a problem hiding this comment.
Still unresolved at 2aaf6938: with_session_pool continues to replace the field via Self { session_pool, ..self }, while shutdown only awaits the replacement. A query/table client created before replacement therefore retains the superseded pool and can keep acquiring sessions after shutdown returns.
— 🤖 AI review on behalf of @rekby
5e45ca9 to
0d259af
Compare
0d259af to
3b26944
Compare
3b26944 to
f60ecbf
Compare
f60ecbf to
97be54b
Compare
97be54b to
ac216fe
Compare
Centralize best-effort DeleteSession execution in a pool-owned worker. Session drops submit cleanup commands, while the worker executes accepted deletions concurrently and drains them when its channel closes.
ac216fe to
2aaf693
Compare
2 participants
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: closes #555, related #560
What is the new behavior?
Other information