Skip to content

feat: add eviction callback in LRU cache #4088

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

DracoLi
Copy link
Contributor

@DracoLi DracoLi commented Jul 15, 2025

Why this should be merged

Adds an optional onEvict callback to the LRU cache to allow resource cleanup when entries are evicted.

x/blockdb (#4027) uses the LRU cache to store file descriptors, so this allows blockdb to properly close the file descriptors when an entry is evicted.

How this works

  • Adds SetOnEvict(callback) method to set an optional cleanup function
  • The callback is called whenever an entry is evicted (during Put, Evict, or Flush)
  • Existing code continues to work unchanged

How this was tested

Unit tests added

Need to be documented in RELEASES.md?

Don't think so

@DracoLi DracoLi marked this pull request as ready for review July 15, 2025 16:16
@Copilot Copilot AI review requested due to automatic review settings July 15, 2025 16:16
@DracoLi DracoLi requested a review from StephenButtolph as a code owner July 15, 2025 16:16
Copilot

This comment was marked as outdated.

@DracoLi DracoLi marked this pull request as draft July 15, 2025 18:10
@DracoLi DracoLi requested a review from Copilot July 15, 2025 19:09
Copilot

This comment was marked as outdated.

@DracoLi DracoLi requested a review from Copilot July 15, 2025 19:22
Copilot

This comment was marked as outdated.

@DracoLi DracoLi requested a review from Copilot July 15, 2025 19:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds an optional eviction callback (onEvict) to the LRU cache API and verifies its behavior in unit tests.

  • Introduces SetOnEvict to register a callback invoked before entries are removed.
  • Updates Put, Evict, and Flush methods to trigger the callback on eviction.
  • Adds tests for eviction and flush scenarios with callbacks.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
cache/lru/cache.go Added onEvict field, SetOnEvict and invocation in Put, Evict, and Flush.
cache/lru/cache_test.go Added tests for flush and put evictions using onEvict.
Comments suppressed due to low confidence (1)

cache/lru/cache.go:73

  • There’s no unit test covering the Evict method when an onEvict callback is set. Consider adding a test to ensure onEvict is called when an entry is explicitly evicted via Evict().
	c.lock.Lock()

@DracoLi DracoLi marked this pull request as ready for review July 15, 2025 20:05
@DracoLi DracoLi requested review from a team and mpignatelli12 and removed request for a team July 15, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant