github-actions[bot] commented on code in PR #4278:
URL: https://github.com/apache/iggy/pull/4278#discussion_r4127026119
##########
core/partitions/src/state_transfer.rs:
##########
@@ -2700,14 +2700,16 @@ where
let write_lock = self.write_lock.clone();
let _guard = write_lock.lock().await;
- if let Some(persistence) = &self.persistence {
- self.start_persistence();
- persistence.drain_with_timeout().await.map_err(|source| {
- PartitionInstallError::SwapIo {
+ if self.persistence.is_some() {
+ // [`IggyPartition::barrier_install_files_locked`] covers original
+ // writers even when no [`crate::PartitionPersistence`] checkpoint
can advance.
+ if let Err(source) = self.barrier_install_files_locked().await {
+ self.fence_install_failure(commit_op);
Review Comment:
warning: `install_state_transfer` fences the partition on any error from
`barrier_install_files_locked`, including a `drain_with_timeout` timeout that
sets no persistence failure. The code it replaced treated that timeout as a
retryable install failure. Fence only when `persistence.failure()` is set, and
leave the timeout retryable.
##########
core/partitions/src/install_backup.rs:
##########
@@ -64,11 +67,41 @@ pub async fn begin(directory: &Path) -> io::Result<()> {
begin_with_storage(directory, &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
Review Comment:
warning: `begin_with_storage` passes an empty `synced_files`, so `link_tree`
never skips the fresh-handle open and sync it does for every hard-linked file.
Pass the paths that `barrier_install_files_locked` proves into `begin`, so the
original-writer proof replaces that sync.
##########
core/partitions/src/persistence.rs:
##########
@@ -157,7 +157,7 @@ impl CheckpointBarrier {
Self::from_future(path, async { Ok(()) })
}
- async fn run(self) -> io::Result<PathBuf> {
+ pub(crate) async fn run(self) -> io::Result<PathBuf> {
Review Comment:
nit: `FileSyncBarrier::run` drops `self.path` when the sync fails, so a
refused install reports only the partition directory and not the file that
failed. Include `self.path` in the error, so the log names the writer that
refused the install.
##########
core/partitions/src/persistence.rs:
##########
@@ -1299,6 +1320,18 @@ impl<S: DurableStorage> PartitionPersistence<S> {
.await
}
Mutation::Truncate { from_op, .. } =>
journal.truncate_from(from_op).await,
+ Mutation::Barrier {
+ barriers,
+ offset_files,
+ ..
+ } => {
+ futures::stream::iter(offset_files.iter().map(Ok::<_,
io::Error>))
Review Comment:
simplification: `Mutation::Barrier` copies the offset-file sync and the
barrier join from `Mutation::Checkpoint` at line 1346. Extract one helper that
syncs the retained offset files and runs the barriers, and call it from both
arms.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -9199,6 +9237,62 @@ mod tests {
assert!(persistence.is_durable_through(1));
}
+ #[cfg(target_os = "linux")]
+ #[compio::test]
+ async fn
given_a_failed_writeback_when_installing_state_transfer_should_refuse_backup_publication()
Review Comment:
nit: The test name promises a failed writeback, but the test injects no such
fault: it relies on `IggyIndexWriter::fsync` on `/dev/null` (line 9268)
failing. Rename it to name that fault, as
`checkpoint_index_sync_failure_fences_before_reclaiming_wal_history` does.
--
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]
