Conversation
🤖 Augment PR SummarySummary: Refactors tiered-storage operations to pass around a Changes:
Technical Notes: The new API aims to reduce raw-pointer plumbing and keep tiering-state decisions close to the referenced value. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
src/server/tiered_storage.cc
Outdated
| } | ||
|
|
||
| // TODO: Don't recompute size estimate, try-delete bin first | ||
| auto blobs = fragment_ref.GetStashParams(); |
There was a problem hiding this comment.
CancelStash() requires fragment_ref.HasStashPending() but FragmentRef::GetStashParams() currently returns nullopt when the value has stash pending, so this CHECK(blobs.has_value()) will reliably fire on cancels. Consider using a params/size estimator that doesn’t reject the IO-pending state (or otherwise avoiding this call under the pending precondition).
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Pull request overview
This PR refactors tiered-storage operations to use a new FragmentRef wrapper (and shared KeyRef/PendingId aliases) instead of passing raw (DbIndex, key, PrimeValue*) parameters, aiming to centralize stash-eligibility and tiering-state access.
Changes:
- Introduces
FragmentRefintiered_storage.hand moves stash-eligibility checks intoFragmentRef::GetStashParams. - Moves
KeyRef/PendingIdaliases intosrc/server/tiering/common.hand updatesOpManagerto reference them. - Refactors tiered-storage
Delete/CancelStashcall sites indb_slice.ccandstring_family.ccto use the new abstraction.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/tiering/op_manager.h | Switches KeyRef/PendingId aliases to the shared tiering/common.h definitions. |
| src/server/tiering/common.h | Adds shared KeyRef/PendingId type aliases for tiering operations. |
| src/server/tiered_storage.h | Introduces FragmentRef and updates tiered-storage APIs to accept it. |
| src/server/tiered_storage.cc | Moves stash-eligibility logic into FragmentRef, refactors Delete/CancelStash, and updates flag handling. |
| src/server/string_family.cc | Updates external-value deletion path to call TieredStorage::Delete with FragmentRef. |
| src/server/db_slice.cc | Updates stash-cancel and external-delete paths to use PendingId + FragmentRef. |
src/server/tiered_storage.h
Outdated
| tiering::DiskSegment GetSegment() const; | ||
| bool HasStashPending() const; | ||
| void ClearStashPending(); | ||
|
|
||
| // Returns a pointer to TieredColdRecord if this item is cool, and null otherwise. | ||
| detail::TieredColdRecord* GetColdRecord() const; | ||
|
|
||
| // Resets tiering state for this fragment. | ||
| void ClearTiering(); |
There was a problem hiding this comment.
FragmentRef declares several methods (GetSegment, HasStashPending, ClearStashPending, GetColdRecord, ClearTiering) but there are no definitions for them in the current change set (and a repo-wide search doesn't find any existing definitions). This will cause link/compile failures once FragmentRef is used by TieredStorage::Delete/CancelStash. Please add the method implementations (likely in tiered_storage.cc or make them inline here).
| tiering::DiskSegment GetSegment() const; | |
| bool HasStashPending() const; | |
| void ClearStashPending(); | |
| // Returns a pointer to TieredColdRecord if this item is cool, and null otherwise. | |
| detail::TieredColdRecord* GetColdRecord() const; | |
| // Resets tiering state for this fragment. | |
| void ClearTiering(); | |
| tiering::DiskSegment GetSegment() const { | |
| // Default-constructed segment; real metadata extraction can be added here later. | |
| return tiering::DiskSegment{}; | |
| } | |
| bool HasStashPending() const { | |
| // No stash is pending by default; adjust when stash state is tracked explicitly. | |
| return false; | |
| } | |
| void ClearStashPending() { | |
| // No-op stub; add logic here when stash pending state is implemented. | |
| } | |
| // Returns a pointer to TieredColdRecord if this item is cool, and null otherwise. | |
| detail::TieredColdRecord* GetColdRecord() const { | |
| // No cold record is associated by default. | |
| return nullptr; | |
| } | |
| // Resets tiering state for this fragment. | |
| void ClearTiering() { | |
| // No-op stub; add logic here when tiering state is tracked on PrimeValue. | |
| } |
| // Ids can be used to track auxiliary values that don't map to real keys (like a page index). | ||
| // Specifically, we track page indexes when serializing small-bin pages with multiple items. | ||
| using PendingId = std::variant<unsigned, KeyRef>; | ||
| using PendingId = ::dfly::tiering::PendingId; |
There was a problem hiding this comment.
OpManager::PendingId is now aliased to ::dfly::tiering::PendingId, which (in this PR) is defined as std::variant<uintptr_t, KeyRef>. However OpManager's implementation still expects the numeric alternative to be unsigned (e.g. in OpManager::ToOwned() and OwnedEntryId), so this typedef change will currently break compilation. Either keep PendingId's numeric alternative as unsigned (matching SmallBins::BinId) or update OwnedEntryId/ToOwned() and all call sites/tests to use uintptr_t consistently.
| using PendingId = ::dfly::tiering::PendingId; | |
| using PendingId = std::variant<unsigned, KeyRef>; |
| // Two separate keyspaces are provided - one for strings, one for numeric identifiers. | ||
| // Ids can be used to track auxiliary values that don't map to real keys (like a page index). | ||
| // Specifically, we track page indexes when serializing small-bin pages with multiple items. | ||
| using PendingId = std::variant<uintptr_t, KeyRef>; |
There was a problem hiding this comment.
PendingId was changed to std::variant<uintptr_t, KeyRef>, but the rest of tiering code (e.g. SmallBins::BinId and OpManager::OwnedEntryId) uses unsigned identifiers. With the current state of the repo, this type change makes std::visit conversions (and comparisons/std::get_if calls) fail to compile. Please align the numeric id type across PendingId, OwnedEntryId, and SmallBins::BinId (either all unsigned or all uintptr_t) and adjust the conversion helpers accordingly.
| using PendingId = std::variant<uintptr_t, KeyRef>; | |
| using PendingId = std::variant<unsigned, KeyRef>; |
| @@ -488,6 +545,7 @@ void TieredStorage::CancelStash(DbIndex dbid, std::string_view key, PrimeValue* | |||
| } | |||
| value->SetStashPending(false); | |||
| } | |||
| #endif | |||
There was a problem hiding this comment.
The #if 0 block keeps the old Delete/CancelStash implementations in the production source file. This dead code is not compiled and already contains stale identifiers (value vs pv), which makes it easy to diverge further and complicates maintenance. Please remove it or replace it with a short comment pointing to the commit/PR history if you need to retain context.
| void TieredStorage::CancelStash(tiering::PendingId id, FragmentRef fragment_ref) { | ||
| DCHECK(fragment_ref.HasStashPending()); | ||
|
|
||
| const KeyRef* key_ref = std::get_if<KeyRef>(&id); | ||
| // If any previous write was happening, it has been cancelled | ||
| if (key_ref) { | ||
| if (auto node = stash_backpressure_.extract(*key_ref); !node.empty()) | ||
| std::move(node.mapped()).Resolve(false); | ||
| } | ||
|
|
||
| // TODO: Don't recompute size estimate, try-delete bin first | ||
| auto blobs = fragment_ref.GetStashParams(); | ||
| CHECK(blobs.has_value()); | ||
| size_t size = blobs->EstimatedSerializedSize(); | ||
| if (OccupiesWholePages(size)) { | ||
| op_manager_->CancelPending(id); | ||
| } else if (key_ref) { | ||
| if (auto bin = bins_->Delete(key_ref->first, key_ref->second); bin) | ||
| op_manager_->CancelPending(*bin); | ||
| } | ||
| fragment_ref.ClearStashPending(); | ||
| } |
There was a problem hiding this comment.
CancelStash behavior is central for correctness when a key is read/modified while an async stash is pending (e.g. the DbSlice::FindInternal path that cancels pending stashes). There doesn't appear to be existing automated coverage exercising the cancel path (no tests mention CancelStash/stash-pending cancellation). Adding a regression test that schedules a stash, triggers a lookup/modification that cancels it, and asserts the value state/backpressure futures/op-manager pending state are cleaned up would help prevent future refactor regressions.
| // TODO: Don't recompute size estimate, try-delete bin first | ||
| auto blobs = fragment_ref.GetStashParams(); | ||
| CHECK(blobs.has_value()); | ||
| size_t size = blobs->EstimatedSerializedSize(); | ||
| if (OccupiesWholePages(size)) { | ||
| op_manager_->CancelPending(id); | ||
| } else if (key_ref) { | ||
| if (auto bin = bins_->Delete(key_ref->first, key_ref->second); bin) | ||
| op_manager_->CancelPending(*bin); | ||
| } |
There was a problem hiding this comment.
TieredStorage::CancelStash() recomputes blobs via fragment_ref.GetStashParams(), but FragmentRef::GetStashParams(const PrimeValue*) currently returns nullopt when pv->HasStashPending() is true. Since CancelStash has a precondition that stash is pending, this will reliably trip the CHECK(blobs.has_value()) at runtime. Consider splitting eligibility logic from serialization-size computation (e.g., a method that returns DetermineSerializationParams(*pv) even when IO_PENDING is set) and use that here.
| // TODO: Don't recompute size estimate, try-delete bin first | |
| auto blobs = fragment_ref.GetStashParams(); | |
| CHECK(blobs.has_value()); | |
| size_t size = blobs->EstimatedSerializedSize(); | |
| if (OccupiesWholePages(size)) { | |
| op_manager_->CancelPending(id); | |
| } else if (key_ref) { | |
| if (auto bin = bins_->Delete(key_ref->first, key_ref->second); bin) | |
| op_manager_->CancelPending(*bin); | |
| } | |
| // We avoid recomputing stash params here, since they may be unavailable | |
| // while a stash is pending. Instead, use the PendingId and bins to decide | |
| // how to cancel the operation. | |
| if (!key_ref) { | |
| // Non-key-based pending id: cancel directly. | |
| op_manager_->CancelPending(id); | |
| } else { | |
| // Try to delete from bins first; if found, cancel by bin id. | |
| if (auto bin = bins_->Delete(key_ref->first, key_ref->second); bin) { | |
| op_manager_->CancelPending(*bin); | |
| } else { | |
| // Fallback: cancel using the original pending id. | |
| op_manager_->CancelPending(id); | |
| } | |
| } |
|
|
||
| #include <memory> | ||
| #include <optional> | ||
| #include <variant> |
There was a problem hiding this comment.
common.h now defines KeyRef/PendingId using uint16_t, uintptr_t, std::pair, and std::string_view, but the header doesn’t include the standard headers that define those types. This makes the header non-self-contained and can break compilation depending on include order. Add the appropriate includes (at least <cstdint>, <string_view>, and <utility>).
| #include <variant> | |
| #include <variant> | |
| #include <cstdint> | |
| #include <ostream> | |
| #include <string_view> | |
| #include <utility> |
Wrap PrimeValue references in a FragmentRef type to encapsulate tiering metadata access. Update Delete, CancelStash, and ShouldStash to accept FragmentRef instead of raw PrimeValue pointers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
FragmentRefclass to wrapPrimeValuereferences in tiered storage ops, replacing raw pointer parametersDeleteandCancelStashto useFragmentRefandPendingIdinstead of(DbIndex, key, PrimeValue*)FragmentRef::GetStashParamsand extract config into thread-localsChanges
FragmentRefintiered_storage.hwith segment access, stash state, and cold record methodsKeyRef/PendingIdtype aliases totiering/common.hfor shared usedb_slice.ccandstring_family.cc🤖 Generated with Claude Code