refactor: extract shared retire queue crate#132
Conversation
skel84
force-pushed
the
feat/extract-retire-queue
branch
from
0c58d3d to
df56171
Compare
Summary by CodeRabbit
WalkthroughA new shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/allocdb-core/src/state_machine.rs (1)
233-236:⚠️ Potential issue | 🟠 MajorFix inconsistent slot comparison operators in reservation-core retirement logic.
The retirement queues use inconsistent comparison operators.
allocdb-coreandquota-coreboth usecurrent_slot <= retire_after_slot(break condition) for operations. However,reservation-coreis internally inconsistent: hold retirement usesretire_after_slot >= request_slotwhile operation retirement usesretire_after_slot > request_slot. This creates an off-by-one semantic difference where holds retire one slot later than operations for the sameretire_after_slotvalue. Alignreservation-corehold and operation retirement to use the same comparison operator as allocdb-core and quota-core.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-core/src/state_machine.rs` around lines 233 - 236, The reservation retirement logic uses two different comparisons (hold retirement uses retire_after_slot >= request_slot while operation retirement uses retire_after_slot > request_slot); make them consistent by changing the hold-retirement comparison to match the operation-retirement comparison (use retire_after_slot > request_slot) so both retire_reservations and retire_operations use the same operator, aligning reservation-core with allocdb-core/quota-core semantics.
🧹 Nitpick comments (2)
crates/allocdb-retire-queue/src/lib.rs (2)
56-65: Consider adding#[must_use]topop_front.
fronthas#[must_use]butpop_frontdoes not. Sincepop_frontreturns anOptionthat callers typically need to inspect, adding the attribute would be consistent and help catch accidental discards.Suggested change
+ #[must_use] pub fn pop_front(&mut self) -> Option<RetireEntry<K, S>> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-retire-queue/src/lib.rs` around lines 56 - 65, Add the #[must_use] attribute to the pop_front method to match front and prevent accidental discarding of its Option return; update the signature for pub fn pop_front(&mut self) -> Option<RetireEntry<K, S>> in the RetireQueue implementation (the method named pop_front) so the compiler warns when callers ignore the returned Option.
68-132: Consider adding a test for empty queue operations.The existing tests cover happy-path and wrap-around scenarios well. Per coding guidelines requesting extensive test coverage, consider adding a test that explicitly verifies
front()andpop_front()returnNoneon a freshly created queue.Suggested test
#[test] fn empty_queue_returns_none() { let mut queue: RetireQueue<u64, u64> = RetireQueue::with_capacity(2); assert_eq!(queue.front(), None); assert_eq!(queue.pop_front(), None); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-retire-queue/src/lib.rs` around lines 68 - 132, Add a new unit test function named empty_queue_returns_none in the tests module that constructs an empty RetireQueue via RetireQueue::with_capacity(2) and asserts that calling front() and pop_front() on the empty queue both return None; place this test alongside the existing queue_round_trips_entries and queue_wraps_without_allocation tests so the behavior of RetireQueue::front and RetireQueue::pop_front on an empty queue is explicitly validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/allocdb-core/src/state_machine.rs`:
- Around line 233-236: The reservation retirement logic uses two different
comparisons (hold retirement uses retire_after_slot >= request_slot while
operation retirement uses retire_after_slot > request_slot); make them
consistent by changing the hold-retirement comparison to match the
operation-retirement comparison (use retire_after_slot > request_slot) so both
retire_reservations and retire_operations use the same operator, aligning
reservation-core with allocdb-core/quota-core semantics.
---
Nitpick comments:
In `@crates/allocdb-retire-queue/src/lib.rs`:
- Around line 56-65: Add the #[must_use] attribute to the pop_front method to
match front and prevent accidental discarding of its Option return; update the
signature for pub fn pop_front(&mut self) -> Option<RetireEntry<K, S>> in the
RetireQueue implementation (the method named pop_front) so the compiler warns
when callers ignore the returned Option.
- Around line 68-132: Add a new unit test function named
empty_queue_returns_none in the tests module that constructs an empty
RetireQueue via RetireQueue::with_capacity(2) and asserts that calling front()
and pop_front() on the empty queue both return None; place this test alongside
the existing queue_round_trips_entries and queue_wraps_without_allocation tests
so the behavior of RetireQueue::front and RetireQueue::pop_front on an empty
queue is explicitly validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d882978-5624-4942-81f1-97a36c84e278
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/allocdb-core/Cargo.tomlcrates/allocdb-core/src/lib.rscrates/allocdb-core/src/state_machine.rscrates/allocdb-retire-queue/Cargo.tomlcrates/allocdb-retire-queue/src/lib.rscrates/quota-core/Cargo.tomlcrates/quota-core/src/lib.rscrates/quota-core/src/retire_queue.rscrates/quota-core/src/state_machine.rscrates/reservation-core/Cargo.tomlcrates/reservation-core/src/lib.rscrates/reservation-core/src/retire_queue.rscrates/reservation-core/src/state_machine.rsscripts/check_repo.sh
💤 Files with no reviewable changes (5)
- crates/quota-core/src/lib.rs
- crates/reservation-core/src/lib.rs
- crates/reservation-core/src/retire_queue.rs
- crates/allocdb-core/src/lib.rs
- crates/quota-core/src/retire_queue.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Write extensive tests for every meaningful behavior change. Favor invariant tests, negative-path tests, recovery tests, and regression tests over shallow happy-path coverage.
Add extensive logging where it materially improves debuggability or operational clarity. Use the right log level:errorfor invariant breaks, corruption, and failed operations that require intervention;warnfor degraded but expected conditions such as overload, lag, or rejected requests;infofor meaningful lifecycle and state-transition events;debugfor detailed execution traces useful in development;traceonly for very high-volume diagnostic detail.
Logging must be structured and purposeful. Do not add noisy logs that obscure signal or hide bugs.
Files:
crates/allocdb-core/src/state_machine.rscrates/reservation-core/src/state_machine.rscrates/quota-core/src/state_machine.rscrates/allocdb-retire-queue/src/lib.rs
🔇 Additional comments (12)
crates/allocdb-retire-queue/Cargo.toml (1)
1-8: LGTM!Clean minimal manifest using workspace inheritance for version, edition, rust-version, and lints. No external dependencies needed for this pure data structure crate.
crates/quota-core/Cargo.toml (1)
7-10: LGTM!Path dependency correctly added for the new shared crate.
crates/reservation-core/Cargo.toml (1)
7-10: LGTM!Path dependency correctly added for the new shared crate.
crates/allocdb-core/Cargo.toml (1)
7-10: LGTM!Path dependency correctly added. The allowlist in
scripts/check_repo.shis updated accordingly to permit this new internal dependency.Cargo.toml (1)
2-9: LGTM!New crate correctly added to workspace members in alphabetical order.
scripts/check_repo.sh (1)
36-55: LGTM!Allowlist correctly extended to include the new internal crate. This maintains the trusted-core dependency restrictions while permitting this specific internal dependency.
crates/allocdb-core/src/state_machine.rs (2)
6-6: LGTM!Import correctly updated to use the new shared crate.
143-144: LGTM!Queue field types correctly updated to use the generic
RetireQueue<K, Slot>form with appropriate type parameters.crates/reservation-core/src/state_machine.rs (2)
8-8: LGTM!The import correctly references the new shared
allocdb_retire_queuecrate, importing the necessary types.
85-86: LGTM!The field types are correctly updated to use the two-parameter generic
RetireQueue<K, S>. Both type instantiations (RetireQueue<HoldId, Slot>andRetireQueue<ClientOperationKey, Slot>) satisfy theCopytrait bounds required by the shared crate, and all usages throughout the file are consistent with the updated API.crates/quota-core/src/state_machine.rs (2)
8-8: LGTM!The import correctly references the new shared
allocdb_retire_queuecrate.
55-55: LGTM!The field type is correctly updated to use the two-parameter generic
RetireQueue<OperationId, Slot>, satisfying theCopytrait bounds required by the shared crate. All usages inretire_operations()andpush_operation_retirement()are consistent with the updated API.
1 participant
Summary
allocdb-retire-queuecrateallocdb-core,quota-core, andreservation-coreto use the shared queue type directlyTesting
Closes
Refs