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

[Do not review] Sync/flush changes for buffered writes #2778

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ashmeenkaur
Copy link
Collaborator

@ashmeenkaur ashmeenkaur commented Dec 10, 2024

Description

This PR includes changes to

  • integrate buffered write handler with sync/flush file flow.
  • Fall back to temp file flow in case of out of order writes.
  • Finalize the object in case of error during write.

Some refactoring is done to reuse the code.

Note: Flush empty file changes required precondition to be propagated to gcs request and will be taken up in the follow up PR. Unit tests are not added for it.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - Added
  3. Integration tests - NA

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 93.61702% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.62%. Comparing base (2838d80) to head (4a9d15c).

Files with missing lines Patch % Lines
internal/fs/inode/file.go 93.61% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2778      +/-   ##
==========================================
+ Coverage   76.51%   76.62%   +0.10%     
==========================================
  Files         116      116              
  Lines       16065    16106      +41     
==========================================
+ Hits        12292    12341      +49     
+ Misses       3257     3251       -6     
+ Partials      516      514       -2     
Flag Coverage Δ
unittests 76.62% <93.61%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashmeenkaur ashmeenkaur marked this pull request as ready for review December 10, 2024 14:35
@ashmeenkaur ashmeenkaur requested a review from a team as a code owner December 10, 2024 14:35
Base automatically changed from updateFlush to master December 11, 2024 05:00
@ashmeenkaur ashmeenkaur force-pushed the sync/flush_changes branch 6 times, most recently from c9e4170 to f60c3aa Compare December 12, 2024 11:05
internal/bufferedwrites/buffered_write_handler.go Outdated Show resolved Hide resolved
internal/fs/inode/file.go Outdated Show resolved Hide resolved
return
}

if f.bwh != nil {
// Finalize the object.
return f.flushBufferedWriteHandlerAndUpdateInode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't flush on sync call.
from LLD:

Sync() can be called multiple times during the lifetime of the file write. Once the resumable upload is finalized we will not be able to append anymore data to it. But the intention of sync() call from the application side, is to persist all the data to the backend device. So we will block the syncFile() call and upload all pending buffers to GCS and free up the free buffers. We will not finalize the upload.

Choose a reason for hiding this comment

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

Hey, I wanted to follow this. Is it possible to get access to the LLD?

internal/fs/inode/file.go Outdated Show resolved Hide resolved
@ashmeenkaur ashmeenkaur changed the title Sync/flush changes for buffered writes [Do not review] Sync/flush changes for buffered writes Dec 13, 2024
@ashmeenkaur ashmeenkaur changed the base branch from master to update_bw_sync_and_flush December 13, 2024 09:37
Base automatically changed from update_bw_sync_and_flush to master December 13, 2024 14:47
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.

3 participants