-
Notifications
You must be signed in to change notification settings - Fork 41
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: Create immutable builder for incremental Cardano DB #2223
Conversation
Test Results 4 files ± 0 52 suites ±0 10m 18s ⏱️ -28s Results for commit 1b1913b. ± Comparison against base commit 90f44cd. This pull request removes 6 and adds 37 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
ea49807
to
8d7a22a
Compare
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Fixed
Show fixed
Hide fixed
563b5bd
to
14d1067
Compare
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.
LGTM 👍
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Outdated
Show resolved
Hide resolved
290570c
to
7a38984
Compare
7a38984
to
fc43569
Compare
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.
LGTM 🚀
Just a quick note, I think the location examples in the openapi.yaml
file should be updated with the example provided in the description of this PR.
The examples for CardanoDatabaseArtifactsLocationsMessagePart
and CardanoDatabaseSnapshotMessage
should be updated with the values from the JSON above.
mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs
Outdated
Show resolved
Hide resolved
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.
LGTM 🚀
I've some doubt on the new methods added to the snapshotter
trait but this can be addressed in another PR.
/// Define the ability to create snapshots. | ||
pub trait Snapshotter: Sync + Send { | ||
/// Create a new snapshot with the given filepath. | ||
fn snapshot_all(&self, filepath: &Path) -> StdResult<OngoingSnapshot>; | ||
|
||
/// Create a new snapshot with the given filepath from a subset of directories and files. | ||
fn snapshot_subset(&self, filepath: &Path, files: Vec<PathBuf>) -> StdResult<OngoingSnapshot>; | ||
|
||
/// Check if the snapshot exists. | ||
fn is_snapshot_exist(&self, filepath: &Path) -> bool; |
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.
If I'm not wrong there's a little grammar error here, it should be does
instead of is
fn is_snapshot_exist(&self, filepath: &Path) -> bool; | |
fn does_snapshot_exist(&self, filepath: &Path) -> bool; |
/// Check if the snapshot exists. | ||
fn is_snapshot_exist(&self, filepath: &Path) -> bool; | ||
|
||
/// Give the full target path for the filepath. | ||
fn get_file_path(&self, filepath: &Path) -> PathBuf; |
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.
Those new methods are difficult to understand and use.
They operate using relative path from a cardano database directory:
- This provide no abstraction for callers as they need to know exactly the structure of a cardano database meaning that the knowledge of it is spread out on all callers.
- This allow incorrect use such as giving an absolute path.
We could refactor the whole trait the following way:
- make public the
TarAppender
trait (maybe rename it) and change it's implementers to be domain based:ImmutableFileFullAppender
instead ofAppenderDirAll
AncilaryAppender
andImmutableTrioAppender
instead ofAppenderEntries
- replace the
snapshot_all
andsnapshot_subset
methods of the snapshotter with a uniquesnapshot
method that take thoseTarAppender
implementation as a parameter.
Note
An alternative would be to introduce a new type instead, maybe an enum, and keep the existing appender system private with a mechanism that construct the appropriate appender for a given new type.
This refactor would allow to remove the needs for snapshotter user to have knowledge of the cardano database inner structure. Simplified usage example:
- before:
if !snapshot_all.is_snapshot_exist(Path::from('immutables_trio-i4378.tar.zst')) {
snapshotter.snapshot_subset(
vec![
PathBuff::from(IMMUTABLE_DIR).join("04378.chunk"),
PathBuff::from(IMMUTABLE_DIR).join("04378.primary"),
PathBuff::from(IMMUTABLE_DIR).join("04378.secondary")
]
);
}
- after:
let appender = ImmutableTrioAppender::immutable(4378);
if !snapshot_all.is_snapshot_exist(&appender) {
snapshotter.snapshot(appender);
}
Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
This temp dir setter is not tested because of the difficulties to observe the behavior
…number` to create a template from a list of `FileUri` - clean `file_uploader` module files - rename ImmutableFilesUploader trait `upload` function to `batch_upload`
…gestArtifactBuilder`
…i` and `MultiFilesUri`
453246b
to
a110e44
Compare
* mithril-aggregator from `0.6.12` to `0.6.13` * mithril-common from `0.4.104` to `0.4.105` * openapi.yaml from `0.1.41` to `0.1.42`
a110e44
to
1b1913b
Compare
Content
The
CardanoDatabaseArtifactBuilder
can build and upload immutables files and digests fileImplement the artifact builder for the Incremental Cardano Database with the target JSON structure:
And the target design:
Pre-submit checklist
Issue(s)
Closes #2151