[TieredStorage] Refactor TieredStorage::new_readonly() code path #195

yhchiang-sol · 2024-03-11T23:20:08Z

Problem

The TieredStorage::new_readonly() function currently has the following
problems:

It opens the file without checking the magic number before checking and loading the footer.
It opens the file twice: first to load the footer, then open again by the reader.

Summary of Changes

This PR refactors TieredStorage::new_readonly() so that it first performs all
checks inside the constructor of TieredReadableFile. The TieredReadableFile
instance is then passed to the proper reader (currently HotStorageReader)
when all checks are passed.

Test Plan

Added a new test to check MagicNumberMismatch.
Existing tiered-storage tests

codecov-commenter · 2024-03-12T00:46:05Z

Codecov Report

Attention: Patch coverage is 96.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (184ba6c) to head (daf43bf).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #195   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         837      837           
  Lines      226898   226932   +34     
=======================================
+ Hits       185871   185953   +82     
+ Misses      41027    40979   -48

accounts-db/src/tiered_storage/hot.rs

accounts-db/src/tiered_storage/file.rs

yhchiang-sol · 2024-03-12T19:58:26Z

The test failure doesn't seem related to the PR. Will rebase after the PR is fully reviewed.


git diff --check | 5m 11s
-- | --
  | Checking remote `origin` for updates to target branch `master`
  | ++git diff origin/master --check --oneline
  | .github/workflows/autolock_bot_PR.txt:26: trailing whitespace.
  | +
  | .github/workflows/autolock_bot_closed_issue.txt:26: trailing whitespace.
  | +
  | .github/workflows/solana-action.yml.txt:1: trailing whitespace.
  | +name : minimal
  | .github/workflows/solana-action.yml.txt:33: trailing whitespace.
  | +
  | .github/workflows/solana-action.yml.txt:41: trailing whitespace.
  | +
  | .github/workflows/solana-action.yml.txt:60: trailing whitespace.
  | +
  | .github/workflows/solana-action.yml.txt:64: trailing whitespace.
  | +      fail-fast: false
  | .github/workflows/solana-action.yml.txt:79: trailing whitespace.
  | +      - name: release artifact
  | .github/workflows/solana-action.yml.txt:87: trailing whitespace.
  | +#            choco install openssl
  | .github/workflows/solana-action.yml.txt:92: trailing whitespace.
  | +        uses: actions/checkout@v2
  | .github/workflows/solana-action.yml.txt:104: trailing whitespace.
  | +
  | .github/workflows/solana-action.yml.txt:113: trailing whitespace.
  | :# Received cancellation signal, interrupting
  | >^^^ +++

yhchiang-sol · 2024-03-13T17:59:28Z

Rebase to address the unrelated CI failure.

accounts-db/src/tiered_storage/file.rs

yhchiang-sol · 2024-03-20T04:32:57Z

Squash and rebase to include recent tiered-storage PRs.

brooksprumo

looks good, just some nits

accounts-db/src/tiered_storage/file.rs

brooksprumo · 2024-03-20T14:10:59Z

accounts-db/src/tiered_storage/footer.rs

    // This field is persisted in the storage but not in this struct.
-    // The number should match FOOTER_MAGIC_NUMBER.
+    // The number should match FILE_MAGIC_NUMBER.
    // pub magic_number: u64,


IMO the magic number is not part of the footer. And thus this part can be removed. I know this'll cause other necessary changes, so no need to change anything in this PR. (And it's pretty low priority, so no need to address this immediately.)

Magic number is not part of the footer as it is commented out. The comment is to point out that there is one additional field after the footer (and the magic_number: u64 is also commented out). Maybe we can find a better place for this comment.

It's not part of the struct, but it is part of FOOTER_TAIL_SIZE, so we sometimes consider the magic number as part of the footer.

It's not part of the struct, but it is part of FOOTER_TAIL_SIZE, so we sometimes consider the magic number as part of the footer.

Aghh, got it. The concept of FOOTER_TAIL_SIZE is that the format of the last FOOTER_TAIL_SIZE bytes in any tiered-storage files will not change so that even if we have new versions of the footer that introduces new fields, we can still safely check the magic number and the format version. The tail also has a field describing the size of the footer.

brooksprumo

brooksprumo · 2024-03-20T17:14:36Z

accounts-db/src/tiered_storage/footer.rs

    // This field is persisted in the storage but not in this struct.
-    // The number should match FOOTER_MAGIC_NUMBER.
+    // The number should match FILE_MAGIC_NUMBER.
    // pub magic_number: u64,


It's not part of the struct, but it is part of FOOTER_TAIL_SIZE, so we sometimes consider the magic number as part of the footer.

…a-xyz#195) #### Problem The TieredStorage::new_readonly() function currently has the following problems: * It opens the file without checking the magic number before checking and loading the footer. * It opens the file twice: first to load the footer, then open again by the reader. #### Summary of Changes This PR refactors TieredStorage::new_readonly() so that it first performs all checks inside the constructor of TieredReadableFile. The TieredReadableFile instance is then passed to the proper reader (currently HotStorageReader) when all checks are passed. #### Test Plan * Added a new test to check MagicNumberMismatch. * Existing tiered-storage tests

yhchiang-sol mentioned this pull request Mar 11, 2024

[TieredStorage] Add AccountsFile::TieredStorage #72

Merged

yhchiang-sol marked this pull request as draft March 11, 2024 23:30

yhchiang-sol marked this pull request as ready for review March 12, 2024 02:57

yhchiang-sol requested a review from brooksprumo March 12, 2024 02:58

brooksprumo reviewed Mar 12, 2024

View reviewed changes

yhchiang-sol requested a review from brooksprumo March 12, 2024 19:58

yhchiang-sol force-pushed the ts-open-refactor branch from ef767f1 to 0c298f5 Compare March 13, 2024 17:59

brooksprumo reviewed Mar 13, 2024

View reviewed changes

accounts-db/src/tiered_storage/file.rs Outdated Show resolved Hide resolved

accounts-db/src/tiered_storage/file.rs Outdated Show resolved Hide resolved

accounts-db/src/tiered_storage/file.rs Outdated Show resolved Hide resolved

yhchiang-sol requested a review from brooksprumo March 15, 2024 17:06

yhchiang-sol force-pushed the ts-open-refactor branch from 0c7cf9c to a4762f2 Compare March 20, 2024 04:32

[TieredStorage] Refactor the file open code path

daf43bf

yhchiang-sol force-pushed the ts-open-refactor branch from a4762f2 to daf43bf Compare March 20, 2024 05:01

yhchiang-sol mentioned this pull request Mar 20, 2024

[TieredStorage] Enable file hash in TieredStorage #170

Draft

brooksprumo reviewed Mar 20, 2024

View reviewed changes

+= brooks' comments

6154e5c

yhchiang-sol requested a review from brooksprumo March 20, 2024 17:03

brooksprumo approved these changes Mar 20, 2024

View reviewed changes

yhchiang-sol merged commit 0e932c7 into anza-xyz:master Mar 20, 2024
32 of 37 checks passed

yhchiang-sol deleted the ts-open-refactor branch March 20, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TieredStorage] Refactor TieredStorage::new_readonly() code path #195

[TieredStorage] Refactor TieredStorage::new_readonly() code path #195

yhchiang-sol commented Mar 11, 2024 •

edited

Loading

codecov-commenter commented Mar 12, 2024 •

edited

Loading

yhchiang-sol commented Mar 12, 2024

yhchiang-sol commented Mar 13, 2024

yhchiang-sol commented Mar 20, 2024

brooksprumo left a comment

brooksprumo Mar 20, 2024

yhchiang-sol Mar 20, 2024

brooksprumo Mar 20, 2024

yhchiang-sol Mar 20, 2024

brooksprumo left a comment

brooksprumo Mar 20, 2024

[TieredStorage] Refactor TieredStorage::new_readonly() code path #195

[TieredStorage] Refactor TieredStorage::new_readonly() code path #195

Conversation

yhchiang-sol commented Mar 11, 2024 • edited Loading

Problem

Summary of Changes

Test Plan

codecov-commenter commented Mar 12, 2024 • edited Loading

Codecov Report

yhchiang-sol commented Mar 12, 2024

yhchiang-sol commented Mar 13, 2024

yhchiang-sol commented Mar 20, 2024

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo Mar 20, 2024

Choose a reason for hiding this comment

yhchiang-sol Mar 20, 2024

Choose a reason for hiding this comment

brooksprumo Mar 20, 2024

Choose a reason for hiding this comment

yhchiang-sol Mar 20, 2024

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

brooksprumo Mar 20, 2024

Choose a reason for hiding this comment

yhchiang-sol commented Mar 11, 2024 •

edited

Loading

codecov-commenter commented Mar 12, 2024 •

edited

Loading