chore: adjusments following PR reviews

- make the dependencies builder construct the full FileArchiver
  verification dir path instead of completing it in the archiver itself
- removed `snapshot_subset` from the Snapshotter trait: as it's
  supersede by the new methods that don't need node db knowledge
- removed `FileArchiver::set_verification_temp_dir` as it's never needed
  since this directory is always passed in constructors.
- adjusted some naming (`snapshot` -> `archive` in `file_archiver`
  module)

Author: Alenar

mithril-aggregator/src/artifact_builder/cardano_database_artifacts/digest.rs

@@ -174,15 +174,15 @@ impl DigestArtifactBuilder {
 
         let locations = self.upload_digest_file(&digest_archive).await;
 
-        self.cleanup_uploaded_artefacts(&digest_path, &digest_archive)?;
+        self.cleanup_uploaded_artifacts(&digest_path, &digest_archive)?;
 
         Ok(DigestUpload {
             locations: locations?,
             size: digest_archive.get_uncompressed_size(),
         })
     }
 
-    fn cleanup_uploaded_artefacts(
+    fn cleanup_uploaded_artifacts(
         &self,
         digest_path: &PathBuf,
         digest_archive: &FileArchive,

mithril-aggregator/src/artifact_builder/cardano_database_artifacts/immutable.rs

@@ -677,7 +677,6 @@ mod tests {
             snapshotter
                 .expect_compression_algorithm()
                 .returning(|| CompressionAlgorithm::Gzip);
-            snapshotter.expect_snapshot_subset().never();
 
             create_fake_file(&work_dir.join("00001.tar.gz"), "00001 content");
             create_fake_file(&work_dir.join("00002.tar.gz"), "00002 content");

mithril-aggregator/src/dependency_injection/builder/protocol/artifacts.rs

@@ -95,7 +95,8 @@ impl DependenciesBuilder {
     }
 
     async fn build_file_archiver(&mut self) -> Result<Arc<FileArchiver>> {
-        let archive_verification_directory = std::env::temp_dir();
+        let archive_verification_directory =
+            std::env::temp_dir().join("mithril_archiver_verify_archive");
         let file_archiver = Arc::new(FileArchiver::new(
             self.configuration.zstandard_parameters.unwrap_or_default(),
             archive_verification_directory,

mithril-aggregator/src/services/snapshotter/compressed_archive_snapshotter.rs

@@ -38,19 +38,6 @@ impl Snapshotter for CompressedArchiveSnapshotter {
         self.snapshot(archive_name_without_extension, appender)
     }
 
-    fn snapshot_subset(
-        &self,
-        archive_name_without_extension: &str,
-        entries: Vec<PathBuf>,
-    ) -> StdResult<FileArchive> {
-        if entries.is_empty() {
-            return Err(anyhow!("Can not create snapshot with empty entries"));
-        }
-
-        let appender = AppenderEntries::new(entries, self.db_directory.clone())?;
-        self.snapshot(archive_name_without_extension, appender)
-    }
-
     fn snapshot_ancillary(
         &self,
         immutable_file_number: ImmutableFileNumber,
@@ -151,16 +138,11 @@ impl CompressedArchiveSnapshotter {
         &self,
         immutable_file_number: u64,
     ) -> StdResult<Vec<PathBuf>> {
-        let mut files_to_snapshot = Vec::with_capacity(4);
-
         let next_immutable_file_number = immutable_file_number + 1;
-        files_to_snapshot.extend_from_slice(
-            immutable_trio_names(next_immutable_file_number)
-                .iter()
-                .map(|filename| PathBuf::from(IMMUTABLE_DIR).join(filename))
-                .collect::<Vec<_>>()
-                .as_slice(),
-        );
+        let mut files_to_snapshot: Vec<PathBuf> = immutable_trio_names(next_immutable_file_number)
+            .into_iter()
+            .map(|filename| PathBuf::from(IMMUTABLE_DIR).join(filename))
+            .collect();
 
         let db_ledger_dir = self.db_directory.join(LEDGER_DIR);
         let ledger_files = LedgerFile::list_all_in_dir(&db_ledger_dir)?;
@@ -276,7 +258,7 @@ mod tests {
         let db_directory = test_dir.join("db");
 
         fs::create_dir(&db_directory).unwrap();
-        File::create(&db_directory.join("file_to_archive.txt")).unwrap();
+        File::create(db_directory.join("file_to_archive.txt")).unwrap();
 
         let snapshotter = CompressedArchiveSnapshotter::new(
             db_directory,

mithril-aggregator/src/services/snapshotter/interface.rs

@@ -1,5 +1,3 @@
-use std::path::PathBuf;
-
 use mithril_common::entities::{CompressionAlgorithm, ImmutableFileNumber};
 use mithril_common::StdResult;
 
@@ -11,13 +9,6 @@ pub trait Snapshotter: Sync + Send {
     /// Create a new snapshot with the given filepath.
     fn snapshot_all(&self, archive_name_without_extension: &str) -> StdResult<FileArchive>;
 
-    /// Create a new snapshot with the given filepath from a subset of directories and files.
-    fn snapshot_subset(
-        &self,
-        archive_name_without_extension: &str,
-        files: Vec<PathBuf>,
-    ) -> StdResult<FileArchive>;
-
     /// Create a new snapshot of ancillary files.
     ///
     /// Ancillary files include the last, uncompleted, immutable trio and the last ledger file.

mithril-aggregator/src/services/snapshotter/test_doubles.rs

@@ -60,14 +60,6 @@ impl Snapshotter for DumbSnapshotter {
         Ok(snapshot)
     }
 
-    fn snapshot_subset(
-        &self,
-        archive_name_without_extension: &str,
-        _files: Vec<PathBuf>,
-    ) -> StdResult<FileArchive> {
-        self.snapshot_all(archive_name_without_extension)
-    }
-
     fn snapshot_ancillary(
         &self,
         _immutable_file_number: ImmutableFileNumber,
@@ -133,14 +125,6 @@ impl Snapshotter for FakeSnapshotter {
         ))
     }
 
-    fn snapshot_subset(
-        &self,
-        archive_name_without_extension: &str,
-        _files: Vec<PathBuf>,
-    ) -> StdResult<FileArchive> {
-        self.snapshot_all(archive_name_without_extension)
-    }
-
     fn snapshot_ancillary(
         &self,
         _immutable_file_number: ImmutableFileNumber,
@@ -189,9 +173,11 @@ mod tests {
             assert_eq!(PathBuf::from("archive.tar.gz"), *snapshot.get_file_path());
             assert_eq!(0, snapshot.get_archive_size());
 
-            let snapshot = snapshotter
-                .snapshot_subset("archive", vec![PathBuf::from("whatever")])
-                .unwrap();
+            let snapshot = snapshotter.snapshot_ancillary(3, "archive").unwrap();
+            assert_eq!(PathBuf::from("archive.tar.gz"), *snapshot.get_file_path());
+            assert_eq!(0, snapshot.get_archive_size());
+
+            let snapshot = snapshotter.snapshot_immutable_trio(4, "archive").unwrap();
             assert_eq!(PathBuf::from("archive.tar.gz"), *snapshot.get_file_path());
             assert_eq!(0, snapshot.get_archive_size());
         }
@@ -239,17 +225,6 @@ mod tests {
                     )
                 );
             }
-            {
-                let subset_snapshot = snapshotter
-                    .snapshot_subset("another_whatever", vec![PathBuf::from("subdir")])
-                    .expect("Dumb snapshotter::snapshot_subset should not fail.");
-                assert_eq!(
-                    Some(subset_snapshot),
-                    snapshotter.get_last_snapshot().expect(
-                        "Dumb snapshotter::get_last_snapshot should not fail when some last snapshot."
-                    )
-                );
-            }
         }
     }
 
@@ -307,16 +282,6 @@ mod tests {
                     );
                     assert!(immutable_snapshot.get_file_path().is_file());
                 }
-                {
-                    let subset_snapshot =
-                        fake_snapshotter.snapshot_subset(filename, vec![]).unwrap();
-
-                    assert_eq!(
-                        subset_snapshot.get_file_path(),
-                        &test_dir.join(filename).with_extension("tar.gz")
-                    );
-                    assert!(subset_snapshot.get_file_path().is_file());
-                }
             }
         }
     }

mithril-aggregator/src/tools/file_archiver/api.rs

@@ -74,7 +74,7 @@ impl FileArchiver {
                 if temporary_archive_path.exists() {
                     if let Err(remove_error) = fs::remove_file(&temporary_archive_path) {
                         warn!(
-                            self.logger, " > Post snapshotter.snapshot failure, could not remove temporary archive";
+                            self.logger, " > Post FileArchiver.archive failure, could not remove temporary archive";
                             "archive_path" => temporary_archive_path.display(),
                             "error" => remove_error
                         );
@@ -206,30 +206,28 @@ impl FileArchiver {
             archive.filepath.display()
         );
 
-        let mut snapshot_file_tar = File::open(&archive.filepath).with_context(|| {
+        let mut archive_file_tar = File::open(&archive.filepath).with_context(|| {
             format!(
                 "Verify archive error: can not open archive: '{}'",
                 archive.filepath.display()
             )
         })?;
-        snapshot_file_tar.seek(SeekFrom::Start(0))?;
+        archive_file_tar.seek(SeekFrom::Start(0))?;
 
-        let mut snapshot_archive: Archive<Box<dyn Read>> = match archive.compression_algorithm {
+        let mut tar_archive: Archive<Box<dyn Read>> = match archive.compression_algorithm {
             CompressionAlgorithm::Gzip => {
-                let snapshot_file_tar = GzDecoder::new(snapshot_file_tar);
-                Archive::new(Box::new(snapshot_file_tar))
+                let archive_decoder = GzDecoder::new(archive_file_tar);
+                Archive::new(Box::new(archive_decoder))
             }
             CompressionAlgorithm::Zstandard => {
-                let snapshot_file_tar = Decoder::new(snapshot_file_tar)?;
-                Archive::new(Box::new(snapshot_file_tar))
+                let archive_decoder = Decoder::new(archive_file_tar)?;
+                Archive::new(Box::new(archive_decoder))
             }
         };
 
         let unpack_temp_dir = self
             .verification_temp_dir
-            .join("mithril_archiver_verify_archive")
             // Add the archive name to the directory to allow two verifications at the same time
-            // (useful for tests).
             .join(archive.filepath.file_name().ok_or(anyhow!(
                 "Verify archive error: Could not append archive name to temp directory: archive `{}`",
                 archive.filepath.display(),
@@ -246,7 +244,7 @@ impl FileArchiver {
 
         let verify_result = {
             let mut result = Ok(());
-            for e in snapshot_archive.entries()? {
+            for e in tar_archive.entries()? {
                 match e {
                     Err(e) => {
                         result = Err(anyhow!(e).context("Verify archive error: invalid entry"));
@@ -290,13 +288,6 @@ impl FileArchiver {
 
         Ok(())
     }
-
-    #[cfg(test)]
-    /// Allow to use a custom temporary directory to avoid conflicts during the snapshot verification.
-    pub fn set_verification_temp_dir<T: Into<String>>(&mut self, sub_dir: T) {
-        self.verification_temp_dir =
-            mithril_common::test_utils::TempDir::create("snapshotter-temp", sub_dir);
-    }
 }
 
 #[cfg(test)]
@@ -305,10 +296,8 @@ mod tests {
 
     use mithril_common::test_utils::assert_equivalent;
 
-    use crate::test_tools::TestLogger;
     use crate::tools::file_archiver::appender::{AppenderDirAll, AppenderFile};
     use crate::tools::file_archiver::test_tools::*;
-    use crate::ZstandardCompressionParameters;
 
     use super::*;
 
@@ -344,7 +333,7 @@ mod tests {
     fn should_create_a_valid_archive_with_zstandard_compression() {
         let test_dir =
             get_test_directory("should_create_a_valid_archive_with_zstandard_compression");
-        let target_archive = test_dir.join("archive.tar.gz");
+        let target_archive = test_dir.join("archive.tar.zst");
         let archived_directory = test_dir.join(create_dir(&test_dir, "archived_directory"));
         create_file(&archived_directory, "file_to_archive.txt");
 
@@ -437,39 +426,27 @@ mod tests {
             target_directory: test_dir.clone(),
             compression_algorithm: CompressionAlgorithm::Gzip,
         };
-        let first_snapshot = file_archiver
+        let first_archive = file_archiver
             .archive(
                 archive_params.clone(),
                 AppenderDirAll::new(archived_directory.clone()),
             )
             .unwrap();
-        let first_snapshot_size = first_snapshot.get_archive_size();
+        let first_archive_size = first_archive.get_archive_size();
 
         create_file(&archived_directory, "another_file_to_archive.txt");
 
-        let second_snapshot = file_archiver
+        let second_archive = file_archiver
             .archive(archive_params, AppenderDirAll::new(archived_directory))
             .unwrap();
-        let second_snapshot_size = second_snapshot.get_archive_size();
+        let second_archive_size = second_archive.get_archive_size();
 
-        assert_ne!(first_snapshot_size, second_snapshot_size);
+        assert_ne!(first_archive_size, second_archive_size);
 
-        let unpack_path = second_snapshot.unpack_gzip(&test_dir);
+        let unpack_path = second_archive.unpack_gzip(&test_dir);
         assert!(unpack_path.join("another_file_to_archive.txt").exists());
     }
 
-    #[test]
-    fn can_set_verification_temp_dir_with_str_or_string() {
-        let mut file_archiver = FileArchiver::new(
-            ZstandardCompressionParameters::default(),
-            PathBuf::new(),
-            TestLogger::stdout(),
-        );
-
-        file_archiver.set_verification_temp_dir("sub_dir");
-        file_archiver.set_verification_temp_dir("sub_dir".to_string());
-    }
-
     #[test]
     fn compute_size_of_uncompressed_data_and_archive() {
         let test_dir = get_test_directory("compute_size_of_uncompressed_data_and_archive");
@@ -485,15 +462,15 @@ mod tests {
             target_directory: test_dir.clone(),
             compression_algorithm: CompressionAlgorithm::Gzip,
         };
-        let snapshot = file_archiver
+        let archive = file_archiver
             .archive(
                 archive_params.clone(),
                 AppenderFile::append_at_archive_root(file_path.clone()).unwrap(),
             )
             .unwrap();
 
-        let expected_archive_size = file_size::compute_size_of_path(&snapshot.filepath).unwrap();
-        assert_eq!(expected_archive_size, snapshot.get_archive_size(),);
-        assert_eq!(777, snapshot.get_uncompressed_size());
+        let expected_archive_size = file_size::compute_size_of_path(&archive.filepath).unwrap();
+        assert_eq!(expected_archive_size, archive.get_archive_size(),);
+        assert_eq!(777, archive.get_uncompressed_size());
     }
 }