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

Abstract out AppendVec into AccountsFile enum #29815

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Abstract out AppendVec into AccountsFile enum #29815

merged 1 commit into from
Feb 16, 2023

Conversation

yhchiang-sol
Copy link
Contributor

@yhchiang-sol yhchiang-sol commented Jan 21, 2023

Summary of Changes

This PR abstracts out AppendVec into AccountsFile enum.
This will allow different implementations of AccountsFile that support the AccountsDB.

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Some musings on the name. Cause it'll only be harder to change going forward 😄

runtime/src/accounts_file.rs Outdated Show resolved Hide resolved
runtime/src/accounts_file.rs Outdated Show resolved Hide resolved
runtime/src/accounts_file.rs Show resolved Hide resolved
@yhchiang-sol yhchiang-sol changed the title Abstract out AppendVec into AccountsFileEntry enum Abstract out AppendVec into AccountsFile enum Feb 15, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Wondering about the impl side of this enum. Is an enum the right abstraction here? Maybe a trait makes more sense, and the AppendVec and the new ColdStorage both implement that trait?

runtime/src/accounts_file.rs Outdated Show resolved Hide resolved
runtime/src/accounts_file.rs Show resolved Hide resolved
runtime/src/accounts_file.rs Show resolved Hide resolved
runtime/src/accounts_file.rs Show resolved Hide resolved
@yhchiang-sol
Copy link
Contributor Author

yhchiang-sol commented Feb 15, 2023

Wondering about the impl side of this enum. Is an enum the right abstraction here? Maybe a trait makes more sense, and the AppendVec and the new ColdStorage both implement that trait?

The implementation of the new storage using this enum is in #28790 (I haven't rebased yet so it's still using the old name AccountsFileEntry.)

I think we can keep the enum for now to maximize the flexibility. Once we've finalized the eventual API together with the cache and in-memory accounts representation, we can revisit here whether we want to keep the enum or do a trait instead. Converting a trait should be fast, but I would like to focus on the implementation for now, but without merging this PR I will need to constantly fix merge conflict 😂.

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Lgtm

@yhchiang-sol yhchiang-sol merged commit aeb6df3 into solana-labs:master Feb 16, 2023
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Mar 12, 2023
Abstracts out AppendVec into AccountsFile enum.
This will allow different implementations of AccountsFile that support the AccountsDB.
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