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

feat: configure num threads for flat storage creation #8088

Merged
merged 8 commits into from
Nov 21, 2022
Merged

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Nov 18, 2022

Make number of threads for background flat storage creation configurable. In the node config we set it to 8, but we also allow node owners to modify it if they observe that BP slows down or creation is too slow until FS becomes a requirement. In the tests we limit number of threads to 1.

I think introducing some general structure as ChainConfig makes sense here, as we already use a configuration option save_trie_changes.

I'm open to other opinions how to organize this. In general NearConfig structure looks a bit suspicious to me, because it includes config taken from config.json and many other configs which are just derived from config.

Testing

Checking that parameter is taken correctly by the node.

@Longarithm Longarithm self-assigned this Nov 18, 2022
@Longarithm Longarithm marked this pull request as ready for review November 18, 2022 16:45
@Longarithm Longarithm requested a review from a team as a code owner November 18, 2022 16:45
@Longarithm Longarithm changed the title draft: configure num threads for flat storage creation feat: configure num threads for flat storage creation Nov 18, 2022
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

LGTM

I wonder what our strategy is here for after the migration. Will we keep this config option, or remove it? If we keep it, we should probably add a an entry to the changelog to announce it.

I think introducing some general structure as ChainConfig makes sense here, as we already use a configuration option save_trie_changes.

Seems ok to me.

In general NearConfig structure looks a bit suspicious to me, because it includes config taken from config.json and many other configs which are just derived from config.

I think suspicious is a generous term here. I could think of other words to describe it.^^

This PR is obviously not gonna change anything on that. But we can discuss what our vision is. My gut feeling is we want a simple as possible struct that defines config.json. It should be structured how validators think about the config. The configs we use in nearcore code should not be tied to JSON format at all and be as detailed as we need them to be. Converting from the config.json struct to specific configurations such as ChainConfig should follow a consistent pattern, fn from_config(config: &NearConfig) -> Self for example.

I think the outline from a comment in an issue about documenting this stuff would be good strategy to really clean this up: #8022 (comment)

chain/chain/src/types.rs Outdated Show resolved Hide resolved
@Longarithm
Copy link
Member Author

Longarithm commented Nov 18, 2022

I wonder what our strategy is here for after the migration. Will we keep this config option, or remove it? If we keep it, we should probably add a an entry to the changelog to announce it.

I would keep it because I can imagine other such migrations in the future. Added it to the changelog under [unreleased]. Also renamed parameter to background_migration_threads because background_work_threads looks like more related to any work done in background :)

Please approve it again if you are okay with such naming.

chain/chain/src/types.rs Outdated Show resolved Hide resolved
Longarithm and others added 2 commits November 21, 2022 20:21
Co-authored-by: nikurt <86772482+nikurt@users.noreply.github.com>
@near-bulldozer near-bulldozer bot merged commit 0040e92 into master Nov 21, 2022
@near-bulldozer near-bulldozer bot deleted the fs-config branch November 21, 2022 16:41
nikurt pushed a commit that referenced this pull request Nov 22, 2022
Make number of threads for background flat storage creation configurable. In the node config we set it to 8, but we also allow node owners to modify it if they observe that BP slows down or creation is too slow until FS becomes a requirement. In the tests we limit number of threads to 1.

I think introducing some general structure as `ChainConfig` makes sense here, as we already use a configuration option `save_trie_changes`.

I'm open to other opinions how to organize this. In general `NearConfig` structure looks a bit suspicious to me, because it includes `config` taken from `config.json` and many other configs which are just derived from `config`.

## Testing

Checking that parameter is taken correctly by the node.
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.

4 participants