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

feat(mito): Add cache manager #2488

Merged
merged 15 commits into from
Sep 26, 2023

Conversation

evenyag
Copy link
Contributor

@evenyag evenyag commented Sep 25, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This PR adds a manager CacheManager to cache data for the mito engine. This is the first step to implement a cache component for the engine.

It implements a cache layer AsyncFileReaderCache for parquet's AsyncFileReader trait so we can return our cached metadata to ParquetRecordBatchStreamBuilder. The AsyncFileReaderCache also has local cached metadata. It is unused now but is useful when we implement row group readers that require building ParquetRecordBatchStreamBuilder multiple times.

Now the CacheManager only caches metadata of the parquet file. This might provide less improvement in performance. I'll start refactoring our ParquetReader to support caching other data after this PR is merged.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

@evenyag evenyag marked this pull request as ready for review September 25, 2023 12:35
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #2488 (6ba4559) into develop (7ecfaa2) will decrease coverage by 0.37%.
Report is 2 commits behind head on develop.
The diff coverage is 91.27%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2488      +/-   ##
===========================================
- Coverage    84.98%   84.61%   -0.37%     
===========================================
  Files          724      727       +3     
  Lines       115298   115603     +305     
===========================================
- Hits         97987    97822     -165     
- Misses       17311    17781     +470     

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

Great work! Finally, we have a cache!

src/mito2/src/sst/parquet/reader.rs Show resolved Hide resolved
src/mito2/src/config.rs Outdated Show resolved Hide resolved
src/mito2/src/cache.rs Outdated Show resolved Hide resolved
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

sst_meta_cache_size doesn't take effect for now right?

@evenyag
Copy link
Contributor Author

evenyag commented Sep 26, 2023

sst_meta_cache_size doesn't take effect for now right?

It takes effect. The default value is a non-zero value.

Copy link
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

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

LGTM

@v0y4g3r v0y4g3r added this pull request to the merge queue Sep 26, 2023
Merged via the queue into GreptimeTeam:develop with commit a6116bb Sep 26, 2023
13 checks passed
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* feat: add cache manager

* feat: add cache to reader builder

* feat: add AsyncFileReaderCache

* feat: Impl AsyncFileReaderCache

* chore: move moka dep to workspace

* feat: add moka cache to the manager

* feat: implement parquet meta cache

* test: test cache manager

* feat: consider vec size

* style: fix clippy

* test: fix config api test

* feat: divide cache

* test: test disabling meta cache

* test: fix config api test

* feat: remove meta cache if file is purged
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.

4 participants