-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature/trie values column #2288
Conversation
@@ -144,6 +147,13 @@ namespace kagome::application { | |||
"platform. Proceed at your own risk."); | |||
#endif | |||
|
|||
if (app_config_->enableDbMigration()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to consider separate flag for each type of migration in the future
auto logger = log::createLogger("Migration", "storage"); | ||
SL_INFO(logger, | ||
"Begin trie storate migration to separate nodes and values"); | ||
if (storage.getSpace(Space::kTrieValue)->cursor()->isValid()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to check getSpace result for nullptr and print critical error if it is, same about cursor()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the responsibility of SpacedStorage to ensure it never returns a nullptr, otherwise it would return an optional. Same for cursor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know that? getSpace's descripion says "return a pointer buffer storage for a space" and that's it. So it's class user's responsibility to check a method result. Same for cursor: "return kv iterator". Check how they're handled in store_impl.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we never use null smart pointers, which means if getSpace returns a shared pointer to a space, it's always a pointer to a space, as specified in its description. The way it's handled in store_impl.cpp should be fixed so it doesn't have an unnecessary check.
std::optional<PolkadotTrieCursor::CertainlyValueAndHash> | ||
PolkadotTrieCursorImpl::value_and_hash() const { | ||
if (const auto *search_state = std::get_if<SearchState>(&state_); | ||
search_state != nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check for nullptr, search_state will be checked automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer an explicit check.
…into feature/trie-values-column
…into feature/trie-values-column
Referenced issues
Description of the Change
Possible Drawbacks
Checklist Before Opening a PR
Before you open a Pull Request (PR), please make sure you've completed the following steps and confirm by answering 'Yes' to each item: