Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe indexer and worker registry now support optional approximate-retention settings. Standalone selection records successful routing decisions when KV events are disabled and overlap scoring is enabled. Tests cover overlap, expiry, namespace isolation, and disabled paths. ChangesStandalone approximate routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change adds standalone approximate KV recording with focused expiration and isolation tests; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let routing_decision_hashes = | ||
| book.then(|| (block_hashes.clone(), sequence_hashes.to_vec())); |
There was a problem hiding this comment.
🔍 Unconditional hash clone on the booked hot path
routing_decision_hashes clones block_hashes and sequence_hashes whenever book is true, before the feature gate at mod.rs. In the default use_kv_events=true deployment those clones are never used, so each booked selection pays two Vec<u64> allocations for nothing. Folding the gate into the .then() avoids the clone when standalone recording is disabled.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Adopted — moved the gate into .then() so the clones only happen when standalone approximate recording is actually enabled.
| fn standalone_approximate_retention_config( | ||
| kv_router_config: &crate::config::KvRouterConfig, | ||
| ) -> Option<ApproximateRetentionConfig> { | ||
| if kv_router_config.use_kv_events { | ||
| return None; | ||
| } | ||
| if kv_router_config.overlap_score_credit <= 0.0 { | ||
| return None; | ||
| } | ||
| let ttl = std::time::Duration::from_secs_f64(kv_router_config.router_ttl_secs.max(0.0)); | ||
| Some(ApproximateRetentionConfig::Ttl(PruneConfig { ttl })) | ||
| } |
There was a problem hiding this comment.
🔍 LRU approximate policy ignored in standalone retention
standalone_approximate_retention_config always builds ApproximateRetentionConfig::Ttl, ignoring router_approximate_cache_policy. lib/kv-router/src/scheduling/config.rs:1043-1047 documents that the lru policy is valid only when use_kv_events=false, yet an operator selecting it here silently gets TTL. This may be an intentional TTL-only first step; confirm.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Standalone mode lacks request-guard lifecycles, so LRU is explicitly downgraded to TTL. Added an info!log when the policy is LRU to make the downgrade visible to operators.
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| let response_isl_tokens = book.then_some(isl_tokens); | ||
| let response_track_prefill_tokens = book.then_some(track_prefill_tokens); | ||
|
|
||
| let routing_decision_hashes = |
There was a problem hiding this comment.
Gating routing_decision_hashes on book records approximate KV state only for /select_and_reserve, so the public /select + /create_reservation flow books KV on a worker without ever adding its prefix to the standalone index. Fix: carry the selected block hashes through PendingSelection/ReservationBooking and record after cached reservation booking succeeds.
🤖 AI Fix
In lib/kv-router/src/services/selection/core/mod.rs and lib/kv-router/src/services/selection/pending.rs, add block_hashes: Vec<LocalBlockHash> to PendingSelection and ReservationBooking, populate it from schedule_selection, and after finalize_reservation successfully books a cached selection call entry.indexer.record_routing_decision(pending.worker, pending.block_hashes.clone(), pending.sequence_hashes.clone()).await? when standalone approximate routing is enabled.
There was a problem hiding this comment.
Fixed. /select now carries block hashes into PendingSelection; /reservations replays them through ReservationBooking, and finalize_reservation records the decision after a successful cached booking.
GitHub Actions
Inactive
GitHub Actions
Inactive
|
👋 Hi flpanbin! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
the
external-contribution
GitHub Actions
Inactive
b0a91f2 to
a8e979f
Compare
GitHub Actions
Inactive
|
The |
tmonty12
left a comment
tmonty12
left a comment
There was a problem hiding this comment.
SelectionService approximate-routing review.
| } | ||
| // Standalone mode has no request-guard lifecycle, so LRU is intentionally | ||
| // downgraded to TTL. Log the downgrade so operators are not surprised. | ||
| if kv_router_config.router_approximate_cache_policy == ApproximateCachePolicyKind::Lru { |
There was a problem hiding this comment.
[P1] router_approximate_cache_policy=lru is an explicit capacity-aware policy, but this silently replaces it with TTL. An info log does not prevent an operator from believing LRU is active.
Please reject this combination in SelectionServiceBuilder::build() before constructing the core: !use_kv_events && overlap_score_credit > 0 && policy == Lru. Keep this validation SelectionService-specific: LRU remains valid for the integrated frontend path.
The reason is not that SelectionService lacks lifecycle callbacks—it has prefill_complete, add_output_block, and free_reservation—but that it does not currently wire them to an ApproximateLruLease and canonical output-block materialization lifecycle.
There was a problem hiding this comment.
Fixed. SelectionServiceBuilder::build() rejects the combination before constructing the core, deriving approximate-mode activation from the shared approximate_retention_config helper so validation can't drift from construction.
| /// (worker-event-disabled) routing. Returns `None` when KV events are enabled | ||
| /// or when overlap scoring is disabled, preserving the existing event-driven | ||
| /// path. | ||
| fn standalone_approximate_retention_config( |
There was a problem hiding this comment.
[nit] standalone_approximate_retention_config describes the current caller/constraint rather than the value this function derives. approximate_retention_config would be clearer and avoids baking “standalone” into a general configuration helper.
There was a problem hiding this comment.
Renamed to approximate_retention_config; doc comment now describes the derived value, not the caller.
| let response_isl_tokens = book.then_some(isl_tokens); | ||
| let response_track_prefill_tokens = book.then_some(track_prefill_tokens); | ||
|
|
||
| let should_record_routing_decision = book |
There was a problem hiding this comment.
[P2] This repeats the same !use_kv_events && overlap_score_credit > 0 decision used when constructing approximate retention. The integrated KvRouter constructs the indexer once, then gates request-side writes on indexer.records_routing_decisions().
Could this path make the constructed indexer’s recording capability authoritative as well? A config helper can centralize construction intent, but the actual recording decision should follow the indexer that was built so the two paths cannot drift.
There was a problem hiding this comment.
Fixed. The service Indexer now carries a records_routing_decisions flag, exposed as can_record_routing_decisions(). Both write paths gate on entry.indexer.can_record_routing_decisions() instead of re-reading config. Construction intent stays in approximate_retention_config, so the two can't drift.
a8e979f to
da31b86
Compare
GitHub Actions
Inactive
da31b86 to
32349e1
Compare
GitHub Actions
Inactive
32349e1 to
bc4c0e4
Compare
GitHub Actions
Inactive
bc4c0e4 to
ae94cb3
Compare
ae94cb3 to
33d36be
Compare
Signed-off-by: bin <bin.pan@daocloud.io>
33d36be to
6e724cb
Compare
This branch was successfully deployed
2 participants
Overview:
When
DYN_USE_KV_EVENTS=false, the standalone embedded EPP routing path had no way to learn about KV blocks produced by workers. As a result, repeated prefixes were scheduled purely load-aware and never saw KV overlap.This change records the selected worker and prompt-block decision after each successful
select_and_reserveinto a local approximate KV index with TTL pruning, so later requests can exploit estimated overlap without worker KV-event ingestion.Details:
Enable standalone approximate retention
WorkerRegistrynow carries an optionalApproximateRetentionConfig; in standalone mode it is set to TTL usingrouter_ttl_secs.create_indexer_with_retention()passes the retention into the primary indexer (KvIndexerorThreadPoolIndexer).Indexer::record_routing_decision()exposes the recording operation to callers.Record successful routing decisions
SelectionCore::schedule_selection()capturesblock_hashes/sequence_hashesfrom the prepared inputs and records them against the selected worker after a successfulselect_and_reserve.!use_kv_events && overlap_score_credit > 0, leaving the event-driven path unchanged.Tests
repeated_prefix_without_kv_events_uses_approximate_overlapapproximate_overlap_decisions_expire_after_ttlapproximate_routing_is_isolated_by_cache_namespaceapproximate_routing_is_disabled_when_kv_events_enabledapproximate_r_is_disabled_when_overlap_credit_zeroWhere should the reviewer start?
lib/kv-router/src/services/selection/core/mod.rs—standalone_approximate_retention_config()and theschedule_selection()recording logic.lib/kv-router/src/services/indexer/backend.rs—Indexer::record_routing_decision()andcreate_indexer_with_retention().lib/kv-router/src/services/selection/tests.rs— focused tests for the new standalone approximate behavior.Validation
Unit tests
cargo test -p dynamo-kv-router --features standalone-selection cargo clippy -p dynamo-kv-router --features standalone-selection cargo check -p dynamo-ext-procEnd-to-end test (agentgateway + standalone EPP + vLLM worker)
Deployed with
DYN_USE_KV_EVENTS=falseand defaultrouter_ttl_secs=120. Sent the same prompt three times:First request
overlap_credit_blocks: 0.000effective cached blocks: 0.00request (same prompt, a few seconds later)
overlap_credit_blocks: 5.000effective cached blocks: 5.00select_and_reservedecision was recorded; the second request matches it.Third request (same prompt, ~3 minutes later)
overlap_credit_blocks: 0.000effective cached blocks: 0.00End-to-end
cache_saltconfigs:
Results:
overlap_credit_blocks: 0.000, effective cached blocks 0.00overlap_credit_blocks > 0, effective cached blocks > 0overlap_credit_blocks: 0.000overlap_credit_blocks: 0.000overlap_credit_blocks: 0.000Related Issues
🔗 This PR is linked to an issue:
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit
New Features
Bug Fixes