perf: implement session-local runtime reachability and retire engine ref - #941
Conversation
|
@codex review |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces ChangesEngine and runtime ownership
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant EngineCore
participant TransactionSystem
participant Catalog
Client->>Session: start operation
Session->>EngineCore: admit runtime and resolve capabilities
Session->>TransactionSystem: claim transaction operation
TransactionSystem->>Catalog: execute catalog or table operation
Catalog-->>TransactionSystem: return operation result
TransactionSystem-->>Session: update runtime operation state
Session-->>Client: return result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -15 |
| Duplication | 44 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Greptile SummaryThe PR replaces engine-wide runtime references and session-registry hot-path lookups with session-local runtime reachability while preserving lifecycle admission and terminal ownership.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible follow-up scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/engine.rs | Splits owner coordination from shared runtime capabilities and preserves lifecycle admission and reverse component teardown. |
| doradb-storage/src/session.rs | Adds exact-state weak and strong session runtime types while retaining stable operation, observer, close, abandonment, and pointer-exact removal semantics. |
| doradb-storage/src/trx/mod.rs | Refactors transaction checkout and terminal ownership around SessionRuntime while preserving operation-key, transaction-ID, lock-release, and cleanup checks. |
| doradb-storage/src/trx/sys.rs | Carries exact session runtime authority through abandoned and failed-precommit cleanup submission. |
| doradb-storage/src/component.rs | Centralizes typed pool capabilities and canonical pool guards without changing component shutdown dependencies. |
| doradb-storage/src/catalog/index.rs | Narrows index DDL capability access while accepted execution remains backed by session runtime ownership. |
| doradb-storage/src/recovery/resources.rs | Makes recovery retain independent pool capabilities and guards rather than relying on engine-wide references. |
| doradb-storage/src/index/owned_stream.rs | Retains only the pool guards required by owned index-stream operations. |
Reviews (2): Last reviewed commit: "resolve task" | Re-trigger Greptile
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
doradb-storage/src/engine.rs (1)
517-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the weak registry field to separate it from the strong owner field.
EngineCore::session_registryis aWeak<SessionRegistry>, andEngineInner::session_registryis anArc<SessionRegistry>.EngineInneralso derefs toEngineCore. Field access prefers the inherent field, soinner.session_registryresolves to the strongArcand onlyinner.core.session_registryreaches theWeak. The two identically named fields with opposite ownership strength make that distinction invisible at each call site, and the doc comment at Line 499 states the weak edge must never drive operation resolution.Rename the weak field to make the ownership strength explicit at every use.
♻️ Proposed rename
- /// Cold weak back-reference for pointer-exact idle-session removal. - pub(crate) session_registry: Weak<SessionRegistry>, + /// Cold weak back-reference for pointer-exact idle-session removal. + pub(crate) weak_session_registry: Weak<SessionRegistry>,Update the bootstrap initializer accordingly:
- session_registry: Arc::downgrade(&session_registry), + weak_session_registry: Arc::downgrade(&session_registry),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/engine.rs` around lines 517 - 518, Rename the weak registry field on EngineCore from session_registry to an ownership-explicit name such as weak_session_registry, and update its bootstrap initializer and every access accordingly. Leave EngineInner::session_registry unchanged so strong and weak registry references remain clearly distinguishable at call sites.doradb-storage/src/session.rs (1)
817-1005: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one access style for
EngineCore.
SessionRuntimeexposescore()and also implementsDeref<Target = EngineCore>. Call sites then mix both forms, for exampleself.core().cataloginsidecatalog_guardandadmitted.runtime().poisonerinpin_observer. The behavior is identical, so this is only a readability concern. KeepDereffor capability field access and usecore()only where an explicit&EngineCoreis required, or drop one of the two.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/session.rs` around lines 817 - 1005, Standardize EngineCore access in SessionRuntime call sites: retain Deref for direct capability field access, and use core() only when an explicit &EngineCore is required. Update mixed usages such as catalog_guard and pin_observer without changing behavior, and keep the existing SessionRuntime::core and Deref implementations unless removing one is necessary for consistency.doradb-storage/src/table/persistence.rs (1)
1680-1706: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
engine.maintenance_testdirectly in retry hook calls.
session.engine()returns theEngineCore, while these listeners are already bound fromsession.runtime()asengine. Useengine.maintenance_testfor the hook call inside the existing runtime borrow to avoid a second accessor invocation inobserve_active_root_retryandobserve_frozen_page_retry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doradb-storage/src/table/persistence.rs` around lines 1680 - 1706, Update the test hook call in observe_active_root_retry to pass engine.maintenance_test directly instead of calling session.engine().maintenance_test. Apply the same change in observe_frozen_page_retry, reusing the existing engine binding obtained from session.runtime().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@doradb-storage/src/engine.rs`:
- Around line 517-518: Rename the weak registry field on EngineCore from
session_registry to an ownership-explicit name such as weak_session_registry,
and update its bootstrap initializer and every access accordingly. Leave
EngineInner::session_registry unchanged so strong and weak registry references
remain clearly distinguishable at call sites.
In `@doradb-storage/src/session.rs`:
- Around line 817-1005: Standardize EngineCore access in SessionRuntime call
sites: retain Deref for direct capability field access, and use core() only when
an explicit &EngineCore is required. Update mixed usages such as catalog_guard
and pin_observer without changing behavior, and keep the existing
SessionRuntime::core and Deref implementations unless removing one is necessary
for consistency.
In `@doradb-storage/src/table/persistence.rs`:
- Around line 1680-1706: Update the test hook call in observe_active_root_retry
to pass engine.maintenance_test directly instead of calling
session.engine().maintenance_test. Apply the same change in
observe_frozen_page_retry, reusing the existing engine binding obtained from
session.runtime().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e74c2af4-d516-4255-9223-c2810fc3023e
⛔ Files ignored due to path filters (8)
docs/architecture.mdis excluded by none and included by nonedocs/backlogs/000175-scalable-shared-resource-lifetime-management.mdis excluded by none and included by nonedocs/engine-component-lifetime.mdis excluded by none and included by nonedocs/lock-system.mdis excluded by none and included by nonedocs/public-error-audit.csvis excluded by!**/*.csvand included by nonedocs/tasks/000255-session-local-runtime-reachability.mdis excluded by none and included by nonedocs/tasks/next-idis excluded by none and included by nonedocs/transaction-system.mdis excluded by none and included by none
📒 Files selected for processing (37)
doradb-storage/src/buffer/evict.rsdoradb-storage/src/buffer/readonly.rsdoradb-storage/src/catalog/checkpoint.rsdoradb-storage/src/catalog/history.rsdoradb-storage/src/catalog/index.rsdoradb-storage/src/catalog/mod.rsdoradb-storage/src/catalog/storage/columns.rsdoradb-storage/src/catalog/storage/indexes.rsdoradb-storage/src/catalog/storage/mod.rsdoradb-storage/src/catalog/storage/table_replay_silent_watermarks.rsdoradb-storage/src/catalog/storage/tables.rsdoradb-storage/src/catalog/table.rsdoradb-storage/src/component.rsdoradb-storage/src/engine.rsdoradb-storage/src/index/mod.rsdoradb-storage/src/index/owned_stream.rsdoradb-storage/src/index/row_page_index.rsdoradb-storage/src/log/mod.rsdoradb-storage/src/log/prefix.rsdoradb-storage/src/recovery/mod.rsdoradb-storage/src/recovery/resources.rsdoradb-storage/src/session.rsdoradb-storage/src/table/access.rsdoradb-storage/src/table/gc.rsdoradb-storage/src/table/layout.rsdoradb-storage/src/table/mem_table.rsdoradb-storage/src/table/mod.rsdoradb-storage/src/table/persistence.rsdoradb-storage/src/table/recover.rsdoradb-storage/src/trx/admission.rsdoradb-storage/src/trx/group.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/purge.rsdoradb-storage/src/trx/retention.rsdoradb-storage/src/trx/stmt.rsdoradb-storage/src/trx/sys.rsdoradb-storage/src/trx/sys_trx.rs
💤 Files with no reviewable changes (3)
- doradb-storage/src/log/prefix.rs
- doradb-storage/src/trx/sys_trx.rs
- doradb-storage/src/trx/group.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 93.38% 93.48% +0.09%
==========================================
Files 149 149
Lines 126873 127741 +868
==========================================
+ Hits 118486 119414 +928
+ Misses 8387 8327 -60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closes #940 |
1 participant
Summary by CodeRabbit
Refactor
Tests