Skip to content

feat: read data from write cache#3128

Merged
evenyag merged 6 commits into
GreptimeTeam:mainfrom
QuenKar:feat/query-from-write-cache
Jan 11, 2024
Merged

feat: read data from write cache#3128
evenyag merged 6 commits into
GreptimeTeam:mainfrom
QuenKar:feat/query-from-write-cache

Conversation

@QuenKar
Copy link
Copy Markdown
Contributor

@QuenKar QuenKar commented Jan 10, 2024

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

part of #2965

changes:

  • move fetch ranges function to helper.rs.
  • add read_ranges function for FileCache.
  • support to try reading from write cache first, if failed, read from remote object store directly.
  • tiny refactor usize to u64

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Refer to a related PR or issue link (optional)

#2965

@github-actions github-actions Bot added Size: M docs-not-required This change does not impact docs. labels Jan 10, 2024
@QuenKar QuenKar marked this pull request as ready for review January 10, 2024 07:08
@QuenKar QuenKar force-pushed the feat/query-from-write-cache branch from 4e5c168 to a03037c Compare January 10, 2024 07:11
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 10, 2024

Codecov Report

Attention: 48 lines in your changes are missing coverage. Please review.

Comparison is base (29a7f30) 85.48% compared to head (0d2c646) 85.07%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3128      +/-   ##
==========================================
- Coverage   85.48%   85.07%   -0.42%     
==========================================
  Files         822      822              
  Lines      134403   134634     +231     
==========================================
- Hits       114899   114540     -359     
- Misses      19504    20094     +590     

@QuenKar QuenKar requested review from evenyag and zhongzc January 11, 2024 02:37
@QuenKar QuenKar force-pushed the feat/query-from-write-cache branch from d04d84e to 61e9b74 Compare January 11, 2024 03:01
Comment thread src/mito2/src/sst/parquet/row_group.rs Outdated
Comment thread src/mito2/src/sst/parquet/row_group.rs Outdated
Comment thread src/mito2/src/sst/parquet.rs Outdated
Comment thread src/mito2/src/cache/file_cache.rs
Comment thread src/mito2/src/cache/file_cache.rs Outdated
Comment thread src/mito2/src/sst/parquet/row_group.rs Outdated
Copy link
Copy Markdown
Collaborator

@zhongzc zhongzc left a comment

Choose a reason for hiding this comment

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

most LGTM

@killme2008
Copy link
Copy Markdown
Member

After we merge this PR, could we remove the cache layer?

QuenKar and others added 2 commits January 11, 2024 16:40
Co-authored-by: Yingwen <realevenyag@gmail.com>
@QuenKar
Copy link
Copy Markdown
Contributor Author

QuenKar commented Jan 11, 2024

After we merge this PR, could we remove the cache layer?

couldn't be removed yet, because we doesn't implement cache read file now, if data is not in WriteCache and we still rely on cache layer to cache read data. The Write Cache just caches the write file and upload to remote store.

@QuenKar QuenKar requested a review from evenyag January 11, 2024 09:33
@killme2008
Copy link
Copy Markdown
Member

After we merge this PR, could we remove the cache layer?

couldn't be removed yet, because we doesn't implement cache read file now, if data is not in WriteCache and we still rely on cache layer to cache read data. The Write Cache just caches the write file and upload to remote store.

Got it. I believe our final goal is to merge these two components.

Copy link
Copy Markdown
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

LGTM.

Could we add a case to test the engine with write cache enabled under mito2::engine mod? It could be done in the next PR.

@evenyag evenyag added this pull request to the merge queue Jan 11, 2024
Merged via the queue into GreptimeTeam:main with commit 8ec1e42 Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required This change does not impact docs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants