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: Adds FileHandle to ChunkStream #1255

Merged
merged 8 commits into from
Mar 28, 2023

Conversation

evenyag
Copy link
Contributor

@evenyag evenyag commented Mar 27, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This PR adds the FileHandle to the ChunkStream to avoid the purger purging the file during scan.

The parquet reader actually returns a ChunkStream to scan the file, so we must keep the file handle in the stream

pub async fn chunk_stream(&self) -> Result<ChunkStream> {
let file_path = self.file_handle.file_path();
let operator = self.object_store.clone();

To test this patch, this PR also adds a method to trigger compaction.

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 changed the title Test/read should fail test: read should fail Mar 27, 2023
@evenyag evenyag marked this pull request as ready for review March 27, 2023 08:45
@evenyag evenyag force-pushed the test/read-should-fail branch from d84b253 to 721867e Compare March 27, 2023 08:49
@evenyag evenyag force-pushed the test/read-should-fail branch from 721867e to d7d5910 Compare March 27, 2023 08:51
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.

Do not merge: this is still a work in progress

@evenyag evenyag force-pushed the test/read-should-fail branch from 50a3a13 to 7856eea Compare March 27, 2023 14:44
@evenyag evenyag changed the title test: read should fail fix: Adds FileHandle to ChunkStream Mar 28, 2023
@evenyag evenyag requested review from waynexia and v0y4g3r March 28, 2023 04:04
@evenyag evenyag force-pushed the test/read-should-fail branch from cdcd660 to 1bac539 Compare March 28, 2023 04:50
@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #1255 (1bac539) into develop (0ffa628) will decrease coverage by 0.03%.
The diff coverage is 88.29%.

@@             Coverage Diff             @@
##           develop    #1255      +/-   ##
===========================================
- Coverage    85.64%   85.61%   -0.03%     
===========================================
  Files          495      495              
  Lines        73428    73573     +145     
===========================================
+ Hits         62887    62991     +104     
- Misses       10541    10582      +41     

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.

Skip src/storage/src/region/tests/compact.rs, other parts look good to me

@evenyag evenyag requested a review from MichaelScofield March 28, 2023 08:12
@MichaelScofield MichaelScofield merged commit e72ce5e into GreptimeTeam:develop Mar 28, 2023
@evenyag evenyag deleted the test/read-should-fail branch March 28, 2023 08:22
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* test: Add compaction test

* test: Test read during compaction

* test: Add s3 object store to test

* test: only run compact test

* feat: Hold file handle in chunk stream

* test: check files still exist after compact

* feat: Revert changes to develop.yaml

* test: Simplify MockPurgeHandler
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.

3 participants