[ENH]: Get database name from get_collections_to_gc mcmr endpoint (WIP)#6392
[ENH]: Get database name from get_collections_to_gc mcmr endpoint (WIP)#6392tanujnay112 wants to merge 1 commit intomainfrom
Conversation
tanujnay112
changed the title
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Add database-aware GC listing and orchestrator routing This PR wires the Key Changes• Introduced Possible Issues• Combined single-region + MCMR results bypass the requested This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
rust/sysdb/src/sysdb.rs
Outdated
| let mcmr_collections: Vec<CollectionToGcInfo> = mcmr_res | ||
| .into_inner() | ||
| .collections | ||
| .into_iter() | ||
| .filter_map(|c| c.try_into().ok()) |
There was a problem hiding this comment.
[Reliability] Silent failure on parsing errors: filter_map(|c| c.try_into().ok()) silently drops collections that fail to parse. If there's a data inconsistency or schema mismatch, collections will be missing from the GC list without any error indication, potentially causing memory leaks as those collections are never cleaned up.
Log parsing failures:
let mcmr_collections: Vec<CollectionToGcInfo> = mcmr_res
.into_inner()
.collections
.into_iter()
.filter_map(|c| {
c.try_into()
.map_err(|e| {
tracing::warn!("Failed to parse collection from mcmr_client: {:?}", e);
e
})
.ok()
})
.collect();Context for Agents
Silent failure on parsing errors: `filter_map(|c| c.try_into().ok())` silently drops collections that fail to parse. If there's a data inconsistency or schema mismatch, collections will be missing from the GC list without any error indication, potentially causing memory leaks as those collections are never cleaned up.
Log parsing failures:
```rust
let mcmr_collections: Vec<CollectionToGcInfo> = mcmr_res
.into_inner()
.collections
.into_iter()
.filter_map(|c| {
c.try_into()
.map_err(|e| {
tracing::warn!("Failed to parse collection from mcmr_client: {:?}", e);
e
})
.ok()
})
.collect();
```
File: rust/sysdb/src/sysdb.rs
Line: 16448c1418f to
d6119f8
Compare
OOM Events Detected
|
tanujnay112
force-pushed
the
gc_threading
branch
from
d6119f8 to
c4f36a3
Compare
|
|
||
| // This doesn't respect limit anymore: |
There was a problem hiding this comment.
[Logic] Logic error in limit handling: When mcmr_client is present, the total number of collections returned can exceed the requested limit (up to 2x). The comment acknowledges this but doesn't handle it.
If limit=100, you could get 100 from primary + 100 from mcmr = 200 total, violating the caller's expectation.
if let Some(limit) = req.limit {
let remaining = (limit as usize).saturating_sub(collections.len());
if remaining == 0 {
return Ok(collections);
}
// Adjust req.limit before calling mcmr_client
let mut mcmr_req = req.clone();
mcmr_req.limit = Some(remaining as u64);
match mcmr_client.list_collections_to_gc(mcmr_req).await { /* ... */ }
}Context for Agents
Logic error in limit handling: When `mcmr_client` is present, the total number of collections returned can exceed the requested `limit` (up to 2x). The comment acknowledges this but doesn't handle it.
If `limit=100`, you could get 100 from primary + 100 from mcmr = 200 total, violating the caller's expectation.
```rust
if let Some(limit) = req.limit {
let remaining = (limit as usize).saturating_sub(collections.len());
if remaining == 0 {
return Ok(collections);
}
// Adjust req.limit before calling mcmr_client
let mut mcmr_req = req.clone();
mcmr_req.limit = Some(remaining as u64);
match mcmr_client.list_collections_to_gc(mcmr_req).await { /* ... */ }
}
```
File: rust/sysdb/src/sysdb.rs
Line: 16431 participant
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?