-
Notifications
You must be signed in to change notification settings - Fork 679
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
fix: Create a separate directory for estimates
tables
#2998
Conversation
estimation
tablesestimates
tables
Codecov Report
@@ Coverage Diff @@
## develop #2998 +/- ##
===========================================
+ Coverage 82.62% 82.64% +0.01%
===========================================
Files 242 242
Lines 194443 194452 +9
===========================================
+ Hits 160664 160698 +34
+ Misses 33779 33754 -25
Continue to review full report at Codecov.
|
I think this PR is going to need to add a |
testnet/stacks-node/src/config.rs
Outdated
pub fn get_estimates_path(&self) -> PathBuf { | ||
let mut path = self.get_chainstate_path(); | ||
path.push("estimates"); | ||
assert!(fs::create_dir_all(&path).is_ok()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kantai note the call to create_dir_all
What do you think about this location for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, oops, sorry I missed this -- yeah, this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, I would suggest changing from the assert to an expect:
assert!(fs::create_dir_all(&path).is_ok()); | |
fs::create_dir_all(&path).expect("Failed to create cost estimates path in node chainstate directory."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for the reviews. I added Issue #3000 for the broken test because I think it's unrelated and flaky. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
fixes #2991
chainstate/estimates