Add db set store #4154
Add db set store #4154
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes to the FlexiDag project involve significant updates to the consensus database and type structures. These include new schema definitions, caching mechanisms, and database operations, along with modifications to handle these new features. The updates improve the functionality for reachability data, batch writing, and data caching. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConsensusDb
participant Cache
participant FlexiDagStorage
User->>ConsensusDb: initialize()
ConsensusDb->>Cache: Create CachedDbSetAccess
User->>ConsensusDb: append_child()
ConsensusDb->>Cache: Update children_access
User->>ConsensusDb: insert_future_covering_item()
ConsensusDb->>Cache: Update fcs_access
User->>FlexiDagStorage: write_batch()
FlexiDagStorage->>RocksDB: Write data with batch
User->>FlexiDagStorage: flush_opt(wait)
FlexiDagStorage->>RocksDB: Flush data with options
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
simonjiao
force-pushed
the
add-db-set-store
branch
2 times, most recently
from
eeb2672 to
b6c8dbc
Compare
Benchmark for bdb9978Click to view benchmark
|
simonjiao
force-pushed
the
add-db-set-store
branch
from
b6c8dbc to
0050f48
Compare
Benchmark for 2719b3cClick to view benchmark
|
Benchmark for c631527Click to view benchmark
|
Benchmark for 5377aeeClick to view benchmark
|
Benchmark for 13702dbClick to view benchmark
|
|
@simonjiao: I will review the changes. Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- flexidag/src/consensusdb/access.rs (1 hunks)
- flexidag/src/consensusdb/consensus_reachability.rs (1 hunks)
- flexidag/src/consensusdb/consensus_relations.rs (8 hunks)
- flexidag/src/consensusdb/db.rs (3 hunks)
- flexidag/src/consensusdb/item.rs (1 hunks)
- flexidag/src/consensusdb/mod.rs (1 hunks)
- flexidag/src/consensusdb/set_access.rs (1 hunks)
- flexidag/src/consensusdb/writer.rs (4 hunks)
- storage/src/db_storage/mod.rs (4 hunks)
- storage/src/storage.rs (2 hunks)
- sync/tests/common_test_sync_libs.rs (2 hunks)
Files skipped from review due to trivial changes (5)
- flexidag/src/consensusdb/access.rs
- flexidag/src/consensusdb/consensus_reachability.rs
- flexidag/src/consensusdb/item.rs
- storage/src/storage.rs
- sync/tests/common_test_sync_libs.rs
Additional comments not posted (26)
flexidag/src/consensusdb/mod.rs (1)
12-12: Addition ofset_accessmodule looks good.The new module
set_accessis correctly declared.flexidag/src/consensusdb/writer.rs (4)
11-15: Addition ofput_innermethod toDbWritertrait looks good.The method signature is correct and follows the trait's style.
44-56: Addition ofput_innermethod toDirectDbWriterimplementation looks good.The method correctly follows the
putoperation logic and handles errors appropriately.
83-90: Addition ofput_innermethod toBatchDbWriterimplementation looks good.The method correctly follows the
putoperation logic and handles errors appropriately.
104-111: Addition ofput_innermethod toDbWriterimplementation for&mut Tlooks good.The method correctly delegates the
put_inneroperation.flexidag/src/consensusdb/db.rs (3)
110-112: Addition ofwrite_batchmethod toFlexiDagStoragelooks good.The method correctly delegates the batch write operation and handles errors appropriately.
114-122: Addition ofwrite_batch_optmethod toFlexiDagStoragelooks good.The method correctly delegates the batch write operation with options and handles errors appropriately.
124-129: Addition offlush_optmethod toFlexiDagStoragelooks good.The method correctly delegates the flush operation and handles errors appropriately.
flexidag/src/consensusdb/set_access.rs (6)
15-21: Addition ofDbSetAccess::newmethod looks good.The method correctly initializes the
DbSetAccessstruct.
23-27: Addition ofDbSetAccess::readmethod looks good.The method correctly reads data from the database and handles errors appropriately.
29-43: Addition ofDbSetAccess::writemethod looks good.The method correctly writes data to the database and handles errors appropriately.
45-70: Addition ofDbSetAccess::seek_iteratormethod looks good.The method correctly seeks the iterator in the database and handles errors appropriately.
79-84: Addition ofCachedDbSetAccess::newmethod looks good.The method correctly initializes the
CachedDbSetAccessstruct.
104-121: Addition ofCachedDbSetAccess::writemethod looks good.The method correctly writes data to the database with caching and handles errors appropriately.
flexidag/src/consensusdb/consensus_relations.rs (7)
6-6: Import added: Ensure usage ofCachedDbSetAccess.The import of
CachedDbSetAccessseems appropriate. Verify its correct usage in the file.
33-33: Schema definition modified: Verify correctness.The
RelationChildrenschema definition was changed fromArc<Vec<Hash>>toHash. Ensure that the new definition is correct and compatible with the rest of the code.
63-69: ValueCodec implementation modified: Verify correctness.The
ValueCodecimplementation forRelationChildrenwas changed fromArc<Vec<Hash>>toHash. Ensure that the new implementation is correct and compatible with the rest of the code.
79-79: Field type updated: Verify correctness.The
children_accessfield inDbRelationsStorewas updated to useCachedDbSetAccess. Ensure that the new field type is correct and used appropriately in the struct.
88-88: Constructor updated: Verify correctness.The constructor of
DbRelationsStorewas updated to initialize the newchildren_accessfield type. Ensure that the constructor correctly initializes the new field type.
110-116: Method updated: Verify correctness.The
insert_batchmethod was updated to handle the newchildren_accessfield type. Ensure that the method correctly performs the necessary operations with the new field type.
128-132: Method updated: Verify correctness.The
get_childrenmethod was updated to use the newchildren_accessfield type. Ensure that the method correctly retrieves the children using the new field type.storage/src/db_storage/mod.rs (5)
471-479: Method added: Verify correctness.The
raw_iterator_cf_optmethod was added. Ensure that the method is correctly implemented and used appropriately in the file.
481-483: Method added: Verify correctness.The
raw_iterator_optmethod was added. Ensure that the method is correctly implemented and used appropriately in the file.
Line range hint
485-492:
Method added: Verify correctness.The
raw_get_pinned_cfmethod was added. Ensure that the method is correctly implemented and used appropriately in the file.
497-500: Method added: Verify correctness.The
raw_write_batchmethod was added. Ensure that the method is correctly implemented and used appropriately in the file.
502-510: Methods added: Verify correctness.The
raw_write_batch_optandflush_optmethods were added. Ensure that the methods are correctly implemented and used appropriately in the file.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
flexidag/tests/tests.rs (1)
163-165: Inconsistent return type forget_childrenfunction.The
get_childrenfunction has inconsistent return types across different files. While some instances returnArc<RwLock<Vec<HashValue>>>, others returnBlockHashes. This inconsistency can lead to potential issues.
- Files and Lines to Review:
sync/src/tasks/mock.rs: Line 1flexidag/src/reachability/tests.rs: Line 1flexidag/src/reachability/extensions.rs: Lines 1, 2flexidag/src/reachability/inquirer.rs: Line 1flexidag/src/reachability/reindex.rs: Lines 1, 2, 3, 4, 5, 6, 7, 8flexidag/src/blockdag.rs: Line 1flexidag/src/consensusdb/consensus_relations.rs: Lines 1, 2flexidag/src/consensusdb/consensus_reachability.rs: Lines 1, 2, 3, 4, 5, 6, 7, 8chain/service/src/chain_service.rs: Line 1Please ensure that all instances of
get_childrenreturnArc<RwLock<Vec<HashValue>>>to maintain consistency across the codebase.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
get_childrenmatch the new return type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `get_children` match the new return type. # Test: Search for the function usage. Expect: Only occurances of the new return type. rg --type rust -A 5 $'get_children'Length of output: 13515
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- chain/service/src/chain_service.rs (1 hunks)
- flexidag/src/blockdag.rs (2 hunks)
- flexidag/src/consensusdb/consensus_relations.rs (9 hunks)
- flexidag/src/reachability/relations_service.rs (2 hunks)
- flexidag/tests/tests.rs (2 hunks)
- sync/src/tasks/mock.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- flexidag/src/consensusdb/consensus_relations.rs
Additional comments not posted (5)
flexidag/src/reachability/relations_service.rs (1)
27-29: LGTM!The change to return
Arc<RwLock<Vec<HashValue>>>for better concurrency handling is appropriate.flexidag/src/blockdag.rs (1)
243-248: LGTM!The change to return
Arc<RwLock<Vec<HashValue>>>for better concurrency handling is appropriate.sync/src/tasks/mock.rs (1)
400-400: LGTM!The change to handle read and iteration over block children using
Arc<RwLock<Vec<HashValue>>>is appropriate.chain/service/src/chain_service.rs (1)
451-451: LGTM!The change to handle read and iteration over block children using
Arc<RwLock<Vec<HashValue>>>is appropriate.flexidag/tests/tests.rs (1)
224-233: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
get_childrenmatch the new return type.
Benchmark for dc596bfClick to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
flexidag/src/types/reachability.rs (1)
1-13: Struct definition update: Removal of fieldsThe removal of
childrenandfuture_covering_setfields fromReachabilityDatastruct has not been fully propagated throughout the codebase. The following files still contain references to these fields and need to be updated or removed:
sync/tests/common_test_sync_libs.rs: Lines 1-5sync/src/verified_rpc_client.rs: Lines 1-10sync/src/tasks/mock.rs: Lines 1-10sync/src/tasks/tests.rs: Lines 1-10sync/src/tasks/mod.rs: Lines 1-10sync/src/tasks/block_sync_task.rs: Lines 1-10storage/src/tests/test_block.rs: Lines 1-10storage/src/block/mod.rs: Lines 1-10network-rpc/src/rpc.rs: Lines 1-10network-rpc/api/src/lib.rs: Lines 1-10flexidag/src/blockdag.rs: Lines 1-10flexidag/src/reachability/relations_service.rs: Lines 1-10flexidag/tests/tests.rs: Lines 1-10flexidag/src/ghostdag/protocol.rs: Lines 1-10flexidag/src/reachability/tests.rs: Lines 1-10flexidag/src/reachability/reindex.rs: Lines 1-10flexidag/src/reachability/inquirer.rs: Lines 1-10flexidag/src/consensusdb/consensus_relations.rs: Lines 1-10flexidag/src/consensusdb/consensus_reachability.rs: Lines 1-10flexidag/src/reachability/extensions.rs: Lines 1-10flexidag/src/reachability/tree.rs: Lines 1-10chain/api/src/service.rs: Lines 1-10chain/service/src/chain_service.rs: Lines 1-10commons/forkable-jellyfish-merkle/src/lib.rs: Lines 1-10commons/forkable-jellyfish-merkle/src/jellyfish_merkle_test.rs: Lines 1-10commons/forkable-jellyfish-merkle/src/iterator/mod.rs: Lines 1-10commons/forkable-jellyfish-merkle/src/node_type/node_type_test.rs: Lines 1-10commons/forkable-jellyfish-merkle/src/node_type/mod.rs: Lines 1-10commons/accumulator/src/tree.rs: Lines 1-10Please ensure that all references and manipulations related to these fields are updated or removed in the entire codebase to maintain consistency.
Analysis chain
Struct definition update: Removal of fields
The removal of
childrenandfuture_covering_setfields fromReachabilityDatastruct simplifies the data structure but could have broader implications on data management and access patterns in the system. Ensure that all references and manipulations related to these fields are updated or removed in the entire codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to the removed fields are updated or removed. # Test: Search for the usage of `children` and `future_covering_set`. Expect: No occurrences. rg --type rust -A 5 $'children|future_covering_set'Length of output: 97908
flexidag/src/consensusdb/consensus_reachability.rs (4)
Line range hint
14-55: Introduction of new caching mechanisms and schema definitionsThe addition of new schemas (
ReachabilityChildren,ReachabilityFcs) and caching mechanisms (CachedDbSetAccess) appears to enhance data handling efficiency. Ensure that these new schemas are integrated properly across the system, and consider adding unit tests to validate their behavior and performance.Consider further optimization by exploring fixed-length constants for store prefixes as noted in the TODO comment. This could potentially improve performance in key lookups.
Line range hint
65-141: Codec implementations for new schemasThe implementation of
KeyCodecandValueCodecfor the new schemas is crucial for ensuring data integrity and efficient serialization/deserialization. Review and test these implementations thoroughly to prevent data corruption or performance issues.Consider adding more detailed error handling or logging within these codec implementations to aid in debugging and maintenance.
Line range hint
222-239: Method updates inDbReachabilityStoreThe updates to methods like
append_childandinsert_future_covering_itemto utilize the new caching layers need careful testing to ensure they handle data correctly and efficiently.Would you like me to help in setting up more comprehensive tests for these updated methods?
Line range hint
270-587: Handling of children and future covering sets in reader methodsThe methods
get_childrenandget_future_covering_setnow utilize the new caching layers. It's important to ensure that these methods return accurate and up-to-date data. The TODO comments suggest potential improvements like removing unnecessary cloning, which should be addressed to optimize performance.- Ok(Arc::new(self.children_acess.read(hash)?.read().clone())) + Ok(self.children_acess.read(hash)?.read().clone())This change removes unnecessary use of
Arcwhich could improve performance by reducing memory overhead.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- flexidag/src/consensusdb/consensus_reachability.rs (18 hunks)
- flexidag/src/consensusdb/set_access.rs (1 hunks)
- flexidag/src/types/reachability.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- flexidag/src/consensusdb/set_access.rs
Additional comments not posted (1)
flexidag/src/consensusdb/consensus_reachability.rs (1)
Line range hint
147-209: Updates toDbReachabilityStorewith new access layersThe modifications to
DbReachabilityStoreto handlechildren_accessandfcs_accessare significant. It's essential to ensure that these changes do not disrupt existing functionalities and that they are compatible with the rest of the system's architecture.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- flexidag/src/consensusdb/db.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- flexidag/src/consensusdb/db.rs
Benchmark for 94e2b50Click to view benchmark
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- flexidag/src/consensusdb/consensus_reachability.rs (18 hunks)
- flexidag/src/consensusdb/set_access.rs (1 hunks)
- flexidag/src/types/interval.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- flexidag/src/consensusdb/consensus_reachability.rs
- flexidag/src/consensusdb/set_access.rs
Additional comments not posted (1)
flexidag/src/types/interval.rs (1)
4-4: LGTM! The addition ofPartialOrdandOrdtraits is appropriate.This change allows
Intervalinstances to be used in ordered collections and comparisons.
Benchmark for 6a23ea7Click to view benchmark
|
Benchmark for 71bb247Click to view benchmark
|
simonjiao
force-pushed
the
add-db-set-store
branch
from
b917f34 to
e7e94b1
Compare
Benchmark for 4bb28adClick to view benchmark
|
simonjiao
force-pushed
the
add-db-set-store
branch
2 times, most recently
from
afece09 to
e1d180d
Compare
Benchmark for cc4a9bdClick to view benchmark
|
* mark empty children for new block
1. sort keys before caching
simonjiao
force-pushed
the
add-db-set-store
branch
from
7b8c507 to
b801f80
Compare
Benchmark for 0d76fe2Click to view benchmark
|
simonjiao
force-pushed
the
add-db-set-store
branch
from
c8f2a3a to
5c409e6
Compare
1. fix a typo 2. remove unused codes
simonjiao
force-pushed
the
add-db-set-store
branch
from
5c409e6 to
8cb13cf
Compare
Benchmark for 69ee51dClick to view benchmark
|
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
Enhancements
Intervalstruct to support more operations.Removed
ReachabilityDatastruct, simplifying data structures.New Features
Bug Fixes