Skip to content
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

Flush local filesystem snapshot IO #2483

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Conversation

pcholakov
Copy link
Contributor

@pcholakov pcholakov commented Jan 10, 2025

C/f #2428

Copy link

github-actions bot commented Jan 10, 2025

Test Results

  7 files  ±0    7 suites  ±0   4m 25s ⏱️ +5s
 47 tests ±0   46 ✅ ±0  1 💤 ±0  0 ❌ ±0 
182 runs  ±0  179 ✅ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit d92d5cd. ± Comparison against base commit 483dcd6.

♻️ This comment has been updated with latest results.

@pcholakov pcholakov force-pushed the fix/snapshot-local-fs-failures branch from 01c7c91 to d92d5cd Compare January 10, 2025 14:30
@pcholakov pcholakov requested a review from muhamadazmy January 10, 2025 14:31
@pcholakov pcholakov changed the title Fix/snapshot local fs failures Flush local filesystem snapshot IO Jan 10, 2025
let mut snapshot_file =
tokio::fs::File::create_new(&file_path).await.map_err(|e| {
anyhow!("Failed to create snapshot file {:?}: {}", file_path, e)
})?;
let size = io::copy(&mut file_data, &mut snapshot_file)
.await
.map_err(|e| anyhow!("Failed to download snapshot file {:?}: {}", key, e))?;
snapshot_file.shutdown().await?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might have been a production bug that could have caused files to be truncated on download! Thanks for catching this, @muhamadazmy 🙏

@pcholakov pcholakov marked this pull request as ready for review January 10, 2025 14:34
Copy link
Contributor

@muhamadazmy muhamadazmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢🚀😊

@pcholakov pcholakov merged commit 7c5e614 into main Jan 10, 2025
13 checks passed
@pcholakov pcholakov deleted the fix/snapshot-local-fs-failures branch January 10, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants