[TieredStorage] TieredStorageFile -> TieredReadonlyFile and TieredWritableFIle #260
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #260 +/- ##
=======================================
Coverage 81.9% 81.9%
=======================================
Files 836 836
Lines 226643 226651 +8
=======================================
+ Hits 185636 185650 +14
+ Misses 41007 41001 -6 |
Thank you! Could be. But let's split the struct into two first so that other PRs can proceed. |
#### Problem TieredWritableFile currently uses File instead of BufWriter. This will introduce more syscall when doing file writes. #### Summary of Changes This PR makes TieredWritableFile uses BufWriter to allow the write-call to be more optimized to reduce the number of syscalls. #### Test Plan Existing tiered-storage test. Will run experiments to verify its performance improvement. #### Dependency #260
| @@ -9,10 +9,10 @@ use { | |||
| }; | |||
|
|
|||
| #[derive(Debug)] | |||
| pub struct TieredStorageFile(pub File); | |||
| pub struct TieredReadableFile(pub File); | |||
There was a problem hiding this comment.
nit: The name of the struct is now TieredReadableFile, but the PR title and description both use the incorrect TieredReadonlyFile. It would be preferable for all of these to remain in-sync and correct. It's now too late for the commit message, since the PR has been merged. Please consider this for future PRs.
| impl TieredStorageFile { | ||
| pub fn new_readonly(file_path: impl AsRef<Path>) -> Self { | ||
| impl TieredReadableFile { | ||
| pub fn new(file_path: impl AsRef<Path>) -> Self { |
There was a problem hiding this comment.
Github won't let me comment on line 30, which is my intent
I see below there's a TieredReableFile::new_writable() method. In what cases do we need a reader to create a new writable file?
| pub fn seek(&self, offset: u64) -> IoResult<u64> { | ||
| (&self.0).seek(SeekFrom::Start(offset)) | ||
| } | ||
|
|
||
| pub fn seek_from_end(&self, offset: i64) -> IoResult<u64> { | ||
| (&self.0).seek(SeekFrom::End(offset)) | ||
| } |
There was a problem hiding this comment.
It's too bad these methods are duplicated. Do both the Readable and Writable have calls to these methods? I was under the impression that Writeable does everything in one pass, from top to bottom, and thus would not need to seek.
#### Problem TieredWritableFile currently uses File instead of BufWriter. This will introduce more syscall when doing file writes. #### Summary of Changes This PR makes TieredWritableFile uses BufWriter to allow the write-call to be more optimized to reduce the number of syscalls. #### Test Plan Existing tiered-storage test. Will run experiments to verify its performance improvement. #### Dependency anza-xyz#260
#### Problem TieredWritableFile currently uses File instead of BufWriter. This will introduce more syscall when doing file writes. #### Summary of Changes This PR makes TieredWritableFile uses BufWriter to allow the write-call to be more optimized to reduce the number of syscalls. #### Test Plan Existing tiered-storage test. Will run experiments to verify its performance improvement. #### Dependency anza-xyz/agave#260
Problem
TieredStorageFile struct currently offers new_readonly() and new_writable()
to allow both read and write work-load to share the same struct. However,
as we need the writer to use BufWriter to improve performance as well as
enable Hasher on writes. There is a need to refactor TieredStorageFile to
split its usage for read-only and writable.
Summary of Changes
Refactor TieredStorageFile to TieredReadonlyFIle and TieredWritableFile.
Test Plan
Existing tiered-storage tests.