github-actions[bot] commented on code in PR #4278:
URL: https://github.com/apache/iggy/pull/4278#discussion_r4161268424
##########
core/partitions/src/install_backup.rs:
##########
@@ -60,15 +63,45 @@ pub async fn recover_with_storage<S: DurableStorage>(
/// # Errors
/// Returns an error if the rollback state cannot be made durable.
/// After any failure the caller must stop serving until recovery.
-pub async fn begin(directory: &Path) -> io::Result<()> {
- begin_with_storage(directory, &DiskStorage).await
+pub async fn begin(directory: &Path, synced_files: BTreeSet<PathBuf>) ->
io::Result<()> {
+ begin_with_synced_files(directory, synced_files, &DiskStorage).await
}
+/// Storage-generic form of [`begin`] used by deterministic storage tests.
+///
/// # Errors
/// Returns an error when the install transaction cannot complete durably.
pub async fn begin_with_storage<S: DurableStorage>(
directory: &Path,
storage: &S,
+) -> io::Result<()> {
+ begin_with_synced_files(directory, BTreeSet::new(), storage).await
+}
+
+/// Simulator form of [`begin_with_storage`] that retains original writers in
+/// [`FileSyncBarrier`] values until their durability barriers complete.
+///
+/// # Errors
+/// Returns an error when an original-writer barrier fails or the install
+/// transaction cannot complete durably.
+#[cfg(feature = "simulator")]
+pub async fn begin_with_storage_and_barriers<S: DurableStorage>(
Review Comment:
simplification: `begin_with_storage_and_barriers` differs from
`begin_with_storage` only by the barrier list it runs first. Fold them into one
helper that takes a barrier list, and let the two `begin_with_storage` call
sites pass an empty list.
##########
core/partitions/src/persistence.rs:
##########
@@ -903,6 +912,27 @@ impl<S: DurableStorage> PartitionPersistence<S> {
});
}
+ /// Queue a durability barrier through every retained original writer,
+ /// including offset files restored from the backup, without advancing
+ /// [`PartitionPersistence::checkpoint_op`].
+ pub(crate) fn barrier_files(&self, barriers: Vec<FileSyncBarrier>) ->
BTreeSet<PathBuf> {
+ let synced_files = self
+ .offset_files
+ .borrow()
+ .keys()
+ .map(PathBuf::from)
+ .chain(barriers.iter().map(|barrier| barrier.path.clone()))
+ .collect();
+ let mut offset_files = std::mem::take(&mut
*self.retired_offset_files.borrow_mut());
Review Comment:
simplification: `barrier_files` repeats the take-and-drain pair inside
`checkpoint_files` (line 902) and `retire_offset_files` (line 863). Extract one
helper that drains both collections into a `Vec` and call it from all three.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -1046,13 +1046,52 @@ where
});
return;
}
- barriers.push(CheckpointBarrier::already_synced(writer.path()));
+ // [`IggyIndexWriter::fsync`] used the original descriptor above.
+ // The marker tells [`PartitionPersistence::checkpoint_files`] not
+ // to replace that proof with a fresh-handle sync.
+ barriers.push(FileSyncBarrier::already_synced(writer.path()));
}
let (files, directories) = self.persistence_checkpoint_files(config);
persistence.checkpoint_files(through_op, files, directories, barriers);
self.start_persistence();
}
+ /// Synchronize every live original writer while the caller holds
+ /// [`IggyPartition::write_lock`].
+ ///
+ /// Unlike [`IggyPartition::checkpoint_persistence`], this barrier runs
even
+ /// when the checkpoint frontier cannot advance.
+ pub(crate) async fn barrier_install_files_locked(&self) ->
std::io::Result<BTreeSet<PathBuf>> {
+ let Some(persistence) = &self.persistence else {
Review Comment:
simplification: The `let Some(persistence) = &self.persistence else` guard
cannot fire, because the only caller checks `self.persistence.is_some()` first.
Take `&PartitionPersistence` as a parameter and drop the branch.
##########
core/partitions/src/install_backup.rs:
##########
@@ -60,15 +63,45 @@ pub async fn recover_with_storage<S: DurableStorage>(
/// # Errors
/// Returns an error if the rollback state cannot be made durable.
/// After any failure the caller must stop serving until recovery.
-pub async fn begin(directory: &Path) -> io::Result<()> {
- begin_with_storage(directory, &DiskStorage).await
+pub async fn begin(directory: &Path, synced_files: BTreeSet<PathBuf>) ->
io::Result<()> {
Review Comment:
nit: `link_tree` skips its fresh-handle sync for every path in
`synced_files`, but the doc on `begin()` does not state what that set must
prove. Document that each listed source path is already durable through its
original writer.
##########
core/simulator/src/storage/tests.rs:
##########
@@ -812,12 +814,14 @@ fn
given_a_failed_writeback_when_beginning_an_install_backup_should_refuse_publi
storage.sync_directory(Path::new(DIRECTORY)).await.unwrap();
storage.fail_writeback(path).unwrap();
- let result = install_backup::begin_with_storage(Path::new(DIRECTORY),
&storage).await;
+ let barrier = FileSyncBarrier::from_file(path, writer);
+ let result = install_backup::begin_with_storage_and_barriers(
Review Comment:
nit: This test fails inside the barrier, so `link_tree` never runs with a
non-empty `synced_files`, and the skip this change adds is not exercised here.
Add a case where the barrier succeeds and assert that no fresh-handle sync
lands on a marked path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
