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

[WIP] Add Compressed secondary cache into stress test #10160

Closed
wants to merge 21 commits into from

Conversation

gitbw95
Copy link
Contributor

@gitbw95 gitbw95 commented Jun 14, 2022

Summary:
The secondary cache is randomly disabled or enabled with CompressedSecondaryCache.

Test Plan:

  • To test that the CompressedSecondaryCache is used and the stress test runs successfully, run make -j24 CRASH_TEST_EXT_ARGS=—duration=960 blackbox_crash_test

@gitbw95 gitbw95 requested a review from anand1976 June 14, 2022 04:28
@facebook-github-bot
Copy link
Contributor

@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@gitbw95 gitbw95 changed the title Add Compressed secondary cache into stress test [WIP] Add Compressed secondary cache into stress test Jun 14, 2022
@gitbw95 gitbw95 marked this pull request as draft June 21, 2022 00:01
@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot pushed a commit that referenced this pull request Jul 29, 2022
Summary:
If a secondary cache is configured, its possible that a cache lookup will get a hit in the secondary cache. In that case, the ```LRUCacheShard::Lookup``` doesn't immediately update the ```total_charge``` for the item handle if the ```wait``` parameter is false (i.e caller will call later to check the completeness). However, ```BlockBasedTable::GetEntryFromCache``` assumes the handle is complete and calls ```UpdateCacheHitMetrics```, which checks the usage of the cache item and fails the assert in https://github.com/facebook/rocksdb/blob/main/cache/lru_cache.h#L237 (```assert(total_charge >= meta_charge)```).

To fix this, we call ```UpdateCacheHitMetrics``` later in ```MultiGet```, after waiting for all cache lookup completions.

Test plan -
Run crash test with changes from #10160

Pull Request resolved: #10440

Reviewed By: gitbw95

Differential Revision: D38283968

Pulled By: anand1976

fbshipit-source-id: 31c54ef43517726c6e5fdda81899b364241dd7e1
@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gitbw95 has updated the pull request. You must reimport the pull request before landing.

@gitbw95
Copy link
Contributor Author

gitbw95 commented Jul 29, 2022

This PR is replaced by #10442 .

@gitbw95 gitbw95 closed this Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants