fix: pin eviction guard #5222
fix: pin eviction guard #5222
Conversation
nugaon
force-pushed
the
fix/pin-eviction-guard
branch
from
c54b4b2 to
b22264a
Compare
|
This change may affect other parts of the system and the followings were considered: if pinned chunks remain in the reserve when they should have been evicted then:
because of these points, during the eviction, pinned chunks are protected by retaining the chunk in the chunkstore but it removes all reserve related records. |
| func(item *BatchRadiusItem) { | ||
| eg.Go(func() error { | ||
| err := r.st.Run(ctx, func(s transaction.Store) error { | ||
| return RemoveChunkMetaData(ctx, s, item) |
There was a problem hiding this comment.
Why do we want to remove the Pinned Chunks metadata, would this affect retrieval of those pinned chunks if storage pressure eviction or manual eviction?
Can we add this as a note in migration? |
| evictedItems []*BatchRadiusItem | ||
| pinnedEvictedItems []*BatchRadiusItem |
There was a problem hiding this comment.
What do you think to change names of this 2 collections and add some comment, something like:
nonPinnedItems []*BatchRadiusItem // Fully evicted from reserve + chunkstore
pinnedItems []*BatchRadiusItem // Only evicted from reserve, kept in chunkstore
|
I have tested it with expired batch and seems good. @nugaon if you can, please address other comments. I will approve it now |
|
|
||
| // Check if the chunk is pinned in any collection | ||
| pinned := false | ||
| for _, uuid := range pinUuids { |
There was a problem hiding this comment.
This will slow down batch eviction linearly with the number of collections. Is there a possibility to have collection uuid as a field in BatchRadiusItem so that all of these lookups (Has method calls) are not done? From what I see, it would need some complexity to manage the synchronisations between BatchRadiusItem and pinChunkItem keys, but if the performance degradation is significant, it may be necessary. It is ok to try with this simpler solution.
There was a problem hiding this comment.
that is information redundant approach but faster one for sure.
IMO this is still a good enough approach because if you don't use pinning feature you won't notice any performance difference. It is questionable how much this feature is used to worth modifying the underlying DB structure to be redundant.
The changes implemented comprehensive pin protection for chunk eviction by addressing a critical bug in the
EvictBatchBinmethod.The fix modified the
EvictBatchBinmethod to properly check each chunk against all pin collections using the newly createdpinstore.IsChunkPinnedInCollectionfunction. When a pinned chunk is found, the eviction only removes reserve related records of that chunk. This ensures that non-pinned chunks can still be evicted while maintaining the integrity of pinned content.Reserve capacity eviction is directly protected through the
EvictBatchBinmethod.Batch expiry eviction is automatically protected because the entire flow from
EvictBatchultimately callsEvictBatchBin.Cache eviction was determined to not require pin protection because pinned chunks are stored in the main ChunkStore layer.
Closes #5215
Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):