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

fix: create FlatStorageState inside RuntimeAdapter #7710

Merged
merged 8 commits into from
Sep 28, 2022
Merged

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Sep 28, 2022

We create FlatStorageState on Chain creation, but it currently happens for any RuntimeAdapter impl. Because KeyValueRuntime doesn't and shouldn't support flat storage, it leads to tests failures.

Here we move FSS creation inside RuntimeAdapter impl.

Testing

https://buildkite.com/nearprotocol/nearcore-flat-state/builds/60#018383c5-7838-4e5b-86f1-da2baa381789 - number of failing tests drops from 77 to 4 - which are failing due to other reasons

@Longarithm Longarithm self-assigned this Sep 28, 2022
@matklad
Copy link
Contributor

matklad commented Sep 28, 2022

Because KeyValueRuntime doesn't and shouldn't support flat storage

Could you explain this a bit? KeyValueRuntime has ShardTries, and I thought of flat storage as a sort-of impl detail of ShardTries.

@Longarithm
Copy link
Member Author

KeyValueRuntime has ShardTries, and I thought of flat storage as a sort-of impl detail of ShardTries.

It's not the case. Flat storage is an independent structure, ShardTries only store FlatStateFactory to access flat storage when it is needed.

It is done so because flat storage can be modified outside of ShardTries, from what I remember.

@Longarithm Longarithm marked this pull request as ready for review September 28, 2022 12:05
@Longarithm Longarithm requested a review from a team as a code owner September 28, 2022 12:05
@Longarithm Longarithm requested review from mina86 and mzhangmzz and removed request for mina86 September 28, 2022 12:05
@@ -629,13 +629,11 @@ impl Chain {
// set up flat storage
#[cfg(feature = "protocol_feature_flat_state")]
for shard_id in 0..runtime_adapter.num_shards(&block_head.epoch_id)? {
let flat_storage_state = FlatStorageState::new(
store.store().clone(),
runtime_adapter.create_flat_storage_state_for_shard(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove the feature flag here and add another implementation of create_flat_storage_state_for_shard when the feature is not enabled? I think this creates less code that's gated by the feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't help much, because ChainAccessForFlatStorage needs to be gated.

@near-bulldozer near-bulldozer bot merged commit c72bc7e into master Sep 28, 2022
@near-bulldozer near-bulldozer bot deleted the fs-testing branch September 28, 2022 21:34
nikurt pushed a commit that referenced this pull request Sep 29, 2022
We create FlatStorageState on Chain creation, but it currently happens for any RuntimeAdapter impl. Because KeyValueRuntime doesn't and shouldn't support flat storage, it leads to tests failures.

Here we move FSS creation inside RuntimeAdapter impl.

## Testing

https://buildkite.com/nearprotocol/nearcore-flat-state/builds/60#018383c5-7838-4e5b-86f1-da2baa381789 - number of failing tests drops from 77 to 4 - which are failing due to other reasons
nikurt pushed a commit that referenced this pull request Nov 9, 2022
We create FlatStorageState on Chain creation, but it currently happens for any RuntimeAdapter impl. Because KeyValueRuntime doesn't and shouldn't support flat storage, it leads to tests failures.

Here we move FSS creation inside RuntimeAdapter impl.

## Testing

https://buildkite.com/nearprotocol/nearcore-flat-state/builds/60#018383c5-7838-4e5b-86f1-da2baa381789 - number of failing tests drops from 77 to 4 - which are failing due to other reasons
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.

None yet

3 participants