Skip to content

[TieredStorage] Add capacity() API and limit file size to 16GB #401

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.

Sign up for GitHub

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

Merged
yhchiang-sol merged 2 commits into from
Mar 24, 2024
Merged

[TieredStorage] Add capacity() API and limit file size to 16GB #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ff9615
[TieredStorage] Add capacity() API and limit hot-storage file size to…
yhchiang-sol Mar 22, 2024
a960163
Have HotStorageReader::capacity() simply return len()
yhchiang-sol Mar 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions accounts-db/src/tiered_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ use {

pub type TieredStorageResult<T> = Result<T, TieredStorageError>;

const MAX_TIERED_STORAGE_FILE_SIZE: u64 = 16 * 1024 * 1024 * 1024; // 16 GiB;
Comment thread
brooksprumo marked this conversation as resolved.

/// The struct that defines the formats of all building blocks of a
/// TieredStorage.
#[derive(Clone, Debug, PartialEq)]
Expand Down Expand Up @@ -163,6 +165,11 @@ impl TieredStorage {
pub fn is_empty(&self) -> bool {
self.len() == 0
}

pub fn capacity(&self) -> u64 {
self.reader()
.map_or(MAX_TIERED_STORAGE_FILE_SIZE, |reader| reader.capacity())
Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 22, 2024

Choose a reason for hiding this comment

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

I think this impl makes sense. We don't want to have to specify the max size constant in multiple places (as is currently). So the one in HotStorageReader::capacity() can hopefully be removed.

Copy link
Copy Markdown
Author

@yhchiang-sol yhchiang-sol Mar 23, 2024

Choose a reason for hiding this comment

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

I think here we need MAX_TIERED_STORAGE_FILE_SIZE. Because the or case means we don't have a reader yet, meaning we don't know whether it is cold or hot yet.

But yep, under the HotStorageReader it should return MAX_HOT_STORAGE_FILE_SIZE.

Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 23, 2024

Choose a reason for hiding this comment

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

we don't know whether it is cold or hot yet.

I think this is another example of the API being inverted/backwards. The caller does have a tiered storage though, that's how they can even call capacity(). But the API does not go down into a specific hot/cold impl to ask for its capacity.

Copy link
Copy Markdown
Author

@yhchiang-sol yhchiang-sol Mar 24, 2024

Choose a reason for hiding this comment

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

I think this is another example of the API being inverted/backwards.

My original thinking is that: the accounts-db layer manages temperature (i.e., hot / cold / warm .. etc) as it has the whole picture of active / inactive accounts and other related information like shrink, and the accounts-file layer's job is to determine the best suitable file format based on the temperature assigned by the accounts-db. (and this is the reason why it was TieredHot previously as we eventually need an enum to determine the temperature.)

So back to the main topic, if we think determining temperature is accounts-db layer's job, then we should have some temperature information here even without constructing a reader.

We can revisit this later as we currently don't have plans for cold format. For now I think MAX_TIERED_STORAGE_FILE_SIZE is good enough.

}
}

#[cfg(test)]
Expand Down
4 changes: 4 additions & 0 deletions accounts-db/src/tiered_storage/hot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,10 @@ impl HotStorageReader {
self.len() == 0
}

pub fn capacity(&self) -> u64 {
self.len() as u64
}

/// Returns the footer of the underlying tiered-storage accounts file.
pub fn footer(&self) -> &TieredStorageFooter {
&self.footer
Expand Down
6 changes: 6 additions & 0 deletions accounts-db/src/tiered_storage/readable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ impl TieredStorageReader {
}
}

pub fn capacity(&self) -> u64 {
match self {
Self::Hot(hot) => hot.capacity(),
}
}

/// Returns the footer of the associated HotAccountsFile.
pub fn footer(&self) -> &TieredStorageFooter {
match self {
Expand Down