Skip to content
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

fix: add missing continue to shard cache put #7496

Closed
wants to merge 4 commits into from
Closed

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Aug 29, 2022

If we are unable to remove element with hash taken from deletions queue, we remove LRU item from cache. This is not optimal - we should continue iterating over deletions queue until we don't exhaust it.

Testing

Existing tests - but we will have to add more #7498

@Longarithm Longarithm requested a review from a team as a code owner August 29, 2022 19:15
@Longarithm Longarithm requested a review from mm-near August 29, 2022 19:15
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Do we have some existing tests which cover this scenario? Can we extend them to include assertions about the state of the cache?

@@ -144,6 +144,7 @@ impl TrieCacheInner {
}
None => {
self.metrics.shard_cache_pop_misses.inc();
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! If we already caught a bug here, let's try to compress this experssion to make it simpler and less error prone. Something like this should work I think:

            // First, try to evict value using the key from deletions queue.
            if let Some(key) = self.deletions.pop() {
                match self.cache.pop(&key) {
                    Some(value) => {
                        self.total_size -= value.len() as u64;
                        self.metrics.shard_cache_pop_hits.inc();
                    }
                    None => self.metrics.shard_cache_pop_misses.inc(),
                }
                continue;
            }

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for adding the test (as we shouldn't be in a hurry now).

Copy link
Contributor

@mm-near mm-near left a comment

Choose a reason for hiding this comment

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

please add the test.

@akhi3030
Copy link
Collaborator

akhi3030 commented Nov 7, 2023

I suspect this PR is no longer being actively worked on. Feel free to reopen when you are ready to start working on it again.

@akhi3030 akhi3030 closed this Nov 7, 2023
@Ekleog-NEAR Ekleog-NEAR deleted the sc-continue branch March 29, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants