-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: storage, database #31
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
Conversation
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
ef4e579 to
aebbcdd
Compare
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
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.
Pull Request Overview
This PR adds support for key–value buffer storage by introducing two storage implementations (in-memory and RocksDB) alongside updates to configuration handling and chain specification parsing.
- Reorder and refactor include directives in logger and formatter files
- Introduce RocksDB storage binding and update database configuration parsing
- Add new chain spec implementation and update CMake configurations
Reviewed Changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/log/logger.cpp | Reorders include directives to reference logger header before other includes |
| src/log/formatters/variant.hpp | Updates the visit function call to use jam::visit_in_place instead of kagome version |
| src/injector/node_injector.cpp | Updates storage bindings, switching the default for SpacedStorage to RocksDB |
| src/app/impl/chain_spec_impl.cpp | Implements chain spec loading; genesis header parsing logs errors without propagating |
| CMakeLists.txt | Updates C++ extension settings and adds RocksDB package dependency |
Comments suppressed due to low confidence (1)
src/log/formatters/variant.hpp:25
- [nitpick] Verify that switching from kagome::visit_in_place to jam::visit_in_place is intentional and that the new reference is correctly defined for this use case.
return jam::visit_in_place(variant, [&](const auto &value) {
| }; | ||
| }), | ||
| di::bind<storage::BufferStorage>.to<storage::InMemoryStorage>(), | ||
| //di::bind<storage::SpacedStorage>.to<storage::InMemorySpacedStorage>(), |
Copilot
AI
Jun 18, 2025
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.
[nitpick] Since the binding for InMemorySpacedStorage is commented out, consider either removing the commented code or adding a clarifying comment to explain why RocksDb is now the default binding.
| //di::bind<storage::SpacedStorage>.to<storage::InMemorySpacedStorage>(), | |
| // The binding for InMemorySpacedStorage is commented out because RocksDb is now the default binding for SpacedStorage. | |
| // RocksDb provides better performance and scalability for production environments, while InMemorySpacedStorage is primarily used for testing or lightweight scenarios. | |
| // di::bind<storage::SpacedStorage>.to<storage::InMemorySpacedStorage>(), |
|
|
||
| namespace jam::app { | ||
|
|
||
| namespace pt = boost::property_tree; |
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.
Why not "rapidjson"?
| [[nodiscard]] std::optional<size_t> byteSizeHint() const override; | ||
|
|
||
| private: | ||
| std::map<std::string, ByteVec> storage_; |
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.
std::map<ByteVec, ByteVec>?
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.
For SSO
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.
May use byte2str/str2byte instead of hex/unhex
| } | ||
|
|
||
| private: | ||
| std::map<std::string, ByteVec> entries; |
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.
| std::map<std::string, ByteVec> entries; | |
| std::map<ByteVec, ByteVec> entries; |
| * each operation immediately or as part of a larger batch. | ||
| */ | ||
| template <typename K, typename V> | ||
| struct Writeable { |
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.
Should we copy these interfaces from kagome?
E.g. <K, V> are unused (is multi-inheritance convenient?)
| .openmetricsHttpEndpoint() | ||
| }; | ||
| }), | ||
| di::bind<storage::BufferStorage>.to<storage::InMemoryStorage>(), |
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.
Do you want to provide default in memory cache?
Can components use std::map directly, or interface allows enabling/disabling persistence?
This interface shouldn't be injected, should wrap struct DefaultMemoryCache { shared_ptr<BufferStorage> ptr; }.
| di::bind<storage::BufferStorage>.to<storage::InMemoryStorage>(), |
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.
This is used as current light-weight implementation. Developer will use appropriate implementation on demand. I think, BufferStorage should not be used directly.
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>
This PR add support storages: kv-buffer storage, two implementation: in-memory and rocksdb