fix(epoxy): add list operation#4604
fix(epoxy): add list operation#4604MasterPtato wants to merge 1 commit into04-09-fix_pb-envoy_reduce_round_trips_for_ws_connectionfrom
Conversation
PR Review: fix(epoxy): add list operationState: DRAFT | Author: MasterPtato | +492 / -142 SummaryThis PR adds a range/list operation to the epoxy KV subsystem:
Critical Bugs (Blockers)1. list_optimistic.rs is a broken copy-paste of get_optimistic.rs The function is still named epoxy_kv_get_optimistic. The body references input.key (which does not exist on Input -- which has begin_key/end_key/limit/reverse) and uses Output { value: Some(...) } when Output here has entries: Vec. This file will not compile. The list fan-out logic is entirely unimplemented. 2. KvListRequest is not handled in message_request.rs The variant was added to the RequestKind union in the protocol schema but there is no corresponding match arm in message_request_inner. Nodes receiving a KvListRequest over the wire will hit an unhandled case. High Severity Logic Errors3. cache_begin_key is constructed from end_key (list_local.rs) let cache_begin_key = KvOptimisticCacheKey::new(end_key.to_vec()); // BUG: should be begin_keyThis makes the cache range scan always have begin == end, so cache entries are never returned when include_cache = true. 4. Output ordering is not preserved in list_local_values Entries are collected into a std::collections::HashMap before being returned as a Vec. HashMap iteration order is non-deterministic, discarding the ordering FDB provides. The reverse parameter becomes a no-op in the final output. Use an IndexMap (insertion-order) or sort the output by key before returning. 5. Cache entry deserialized with wrong key type (list_local.rs) In the Either::Right (cache) branch, entries are deserialized using KvValueKey instead of KvOptimisticCacheKey. Compare with get_local.rs which correctly uses cache_key.deserialize(...). This will produce incorrect data or panic. Medium Severity Issues6. limit is not applied to the cache stream The value stream respects limit, but the RangeOption for the cache stream has no limit set. A small limit from the caller will still cause a full cache range scan. 7. CommittedEntry schema type is unused CommittedEntry (key + value) was added to v2.bare but KvListResponse uses list (value only). If callers need to know which key each value belongs to, KvListResponse should use list. Currently CommittedEntry has no references. 8. No legacy subspace fallback in list_local_values read_local_value falls back to LegacyCommittedValueKey for v1 EPaxos data. list_local_values only scans the v2 KvValueKey subspace. During a migration window, list operations will silently omit entries that single-key gets return correctly. Convention Issues9. Glob import from anyhow in list_optimistic.rs use anyhow::*; From CLAUDE.md: "Do not glob import (::*) from anyhow. Instead, import individual types and traits." Use use anyhow::Result; instead. 10. filter_map idiom can be simplified (list_local.rs) // Current
.filter_map(|e| {
if let Some(value) = e.value { Some(Entry { ... }) } else { None }
})
// Preferred
.filter_map(|e| e.value.map(|value| Entry { ... }))11. Doc comments in list_optimistic.rs were copied verbatim and are stale The comments still describe get semantics and contain -- em-dash substitutes. Per CLAUDE.md: "Do not use em dashes. Use periods to separate sentences instead." Test CoverageNo tests are included (PR is draft). Before merging, tests should cover: forward and reverse range scans, key range boundaries, cache merge with include_cache = true (including the cache_begin_key bug as a regression), and the KvListRequest dispatch path. Summary Table
The refactoring of read_value.rs into get_local.rs is clean and the &[u8] signature improvement is a nice change. The rest needs significant work before it can be merged. |
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: