snapshots: SnapshotPath.step_range #2419
Conversation
battlmonstr
force-pushed
the
pr/step
branch
2 times, most recently
from
8208f2e to
16e6864
Compare
battlmonstr
force-pushed
the
pr/step
branch
3 times, most recently
from
9d585f6 to
3fca7c4
Compare
we need to always proceed to download code, because it will only download if needed, and because it will seed them at the same time
battlmonstr
force-pushed
the
pr/step
branch
3 times, most recently
from
39e3453 to
d75dd11
Compare
| StepRange step_range() const { return step_range_; } | ||
| SnapshotType type() const { return type_; } | ||
| std::string type_string() const; | ||
| bool exists() const { return std::filesystem::exists(path_); } |
There was a problem hiding this comment.
Why adding this helper method and removing similar ones for last_write_time and index_file_for_type or snapshot_path_for_type?
I'm fine with this exists helper, but I suggest to keep the existing ones as well.
There was a problem hiding this comment.
Removing path_for_type is not related to steps indeed, but as I wanted to keep SnapshotPath as clean as possible, somehow I couldn't resist doing it in this PR :) I agree that the path_for_type is nice, and I will introduce it later in a different schema-related class and replace direct related_path calls to a higher level API.
The idea is that in the future SnapshotPath will be just a container for meta-data encoded in the filename + the full path. In order to get SnapshotType (and things like index_file_for_type) the code will need to consult "schema" - a new decoupled object. So instead of deducing a schema element from path, we'll normally do the other way around: deduce path(s) from schema. The few cases where we need to go from path to schema will be a new method like schema.type_of_path(path). Removing path_for_type is a preparation for this change.
| snapshots::Index idx_txn_hash{transactions_segment_path->index_file_for_type(snapshots::SnapshotType::transactions), make_region(ts.tx_hash_index)}; | ||
| snapshots::Index idx_txn_hash_2_block{transactions_segment_path->index_file_for_type(snapshots::SnapshotType::transactions_to_block), make_region(ts.tx_hash_2_block_index)}; | ||
| snapshots::Index idx_txn_hash{transactions_segment_path->related_path(snapshots::SnapshotType::transactions, snapshots::kIdxExtension), make_region(ts.tx_hash_index)}; | ||
| snapshots::Index idx_txn_hash_2_block{transactions_segment_path->related_path(snapshots::SnapshotType::transactions_to_block, snapshots::kIdxExtension), make_region(ts.tx_hash_2_block_index)}; |
There was a problem hiding this comment.
IMO code is less readable than before
|
|
||
| auto header_index_builder = snapshots::HeaderIndex::make(header_snapshot_path); | ||
| header_index_builder.set_base_data_id(header_snapshot_file.block_num_range().start); |
There was a problem hiding this comment.
Adding the public method IndexBuilder::set_base_data_id to handle custom start point only for tests is not ideal, but it's necessary to support our sample snapshots used in tests.
A better overall solution would be:
- introduce some subcommand in
snapshotscommand-line utility to generate (short) sample snapshots - use some other command-line utility (maybe new
embed_binary?) to generate C++ code for sample snapshots - regenerate sample snapshots without custom start point (i.e. add initial empty blocks)
- remove
IndexBuilder::set_base_data_idas it becomes unnecessary
I'll open a dedicated issue on this.
No description provided.