Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Fallback to synchronous rm_dir call if path moving fails#27306

Merged
xiangzhu70 merged 1 commit intosolana-labs:masterfrom
xiangzhu70:account_files_remove_fallback
Aug 23, 2022
Merged

Fallback to synchronous rm_dir call if path moving fails#27306
xiangzhu70 merged 1 commit intosolana-labs:masterfrom
xiangzhu70:account_files_remove_fallback

Conversation

@xiangzhu70
Copy link
Copy Markdown
Contributor

Remove some log lines, as suggested in PR #26910

Problem

path renaming may fail in the /mnt/account case.

Summary of Changes

Fall back to synchronous rmdir call if the path renaming fails.

Fixes #

Copy link
Copy Markdown
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread core/src/validator.rs
.name("solDeletePath".to_string())
.spawn(move || {
std::fs::remove_dir_all(&path_delete).unwrap();
info!(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did find this log mesage useful fwiw

Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

lgtm, just a nit. generally please dont ask for review on Draft PRs though. process wise a Draft is something that is not ready for review by definition

Comment thread core/src/validator.rs
err.to_string()
);
std::fs::remove_dir_all(&path).unwrap();
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems like we can easily avoid this early return by using an } else {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I usually let the the function do the early return for various exceptions, and let the later main part of the function represent the more likely flow, and avoid too much indention in that part. I can make the change if less early return is really preferred.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My pref in this case would be to remove the early return but I'm not going to attempt to force compliance.

@xiangzhu70 xiangzhu70 marked this pull request as ready for review August 23, 2022 00:51
@xiangzhu70 xiangzhu70 merged commit 827d8e4 into solana-labs:master Aug 23, 2022
@xiangzhu70 xiangzhu70 deleted the account_files_remove_fallback branch August 23, 2022 13:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants