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

[TieredStorage] HotStorageReader::get_account_offset #33824

Closed

Conversation

yhchiang-sol
Copy link
Contributor

Problem

HotStorageReader currently has get_account_meta_from_offset() as its
internal get account API for given its offset, but how to obtain the offset
of one account is missing.

Summary of Changes

This PR implements HotStorageReader::get_account_offset, which allows
the HotStorageReader to obtain the offset of one account.

Test Plan

A new test is included in this PR.

@yhchiang-sol yhchiang-sol marked this pull request as draft October 24, 2023 06:56
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #33824 (baa8616) into master (bdfb644) will increase coverage by 0.0%.
Report is 4 commits behind head on master.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #33824   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         809      809           
  Lines      217692   217720   +28     
=======================================
+ Hits       178276   178304   +28     
  Misses      39416    39416           

@yhchiang-sol yhchiang-sol marked this pull request as ready for review October 25, 2023 16:56
Comment on lines 231 to 232
/// Returns the offset of the account associated with the specified index.
fn get_account_offset(&self, index: usize) -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you specify offset into what?

Consider also using a newtype to ensure this offset is only used in the correct contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment. The offset to the account in the underlying TieredStorageFile.

As for creating a new type, does TieredStorageFileOffset or simply TieredStorageOffset look good?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me remember, is this returned offset a raw byte-offset from the beginning of the file, or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a raw byte-offset from the beginning of the file (since the accounts block is located at the beginning of a tiered-storage file.)

As for a new type, I have created a #33872 that defines TieredStorageOffset.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a raw byte-offset from the beginning of the file (since the accounts block is located at the beginning of a tiered-storage file.)

Is it an offset from the beginning of the file only because the accounts block is located at the beginning of the stored storage file? IOW, if the accounts storage block was not at the beginning, would the returned offset still be from the beginning of the file, or is it an offset from the beginning of the accounts block?

accounts-db/src/tiered_storage/hot.rs Outdated Show resolved Hide resolved
@yhchiang-sol yhchiang-sol marked this pull request as draft October 30, 2023 17:12
@yhchiang-sol
Copy link
Contributor Author

Created #34031 based on the recent changes.

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.

2 participants