-
Notifications
You must be signed in to change notification settings - Fork 26
Perform reconciliation if all three downstairs are in live-repair #1784
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
base: main
Are you sure you want to change the base?
Conversation
7390c5e to
ffd59f0
Compare
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.
Just a thought on function naming.
I'm excited to see this back, as it unlocks a test I wrote years (and years) ago that I've been waiting to run.
| /// If any of the downstairs is not in `LiveRepairReady` | ||
| #[must_use] | ||
| pub(crate) fn reconcile_from_live_repair_ready(&mut self) -> bool { | ||
| let mut max_flush = 0; |
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.
I'm going to want a stat for DTrace when we do this. Just to see how often it happens. I'm happy to add that myself in a later PR (as I'll be updating the DTrace scripts to print it)
upstairs/src/upstairs.rs
Outdated
| /// | ||
| /// If all Downstairs are in `LiveRepairReady`, we instead begin | ||
| /// reconciliation. | ||
| pub(crate) fn check_live_repair_start(&mut self) { |
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.
I did not reall like the name of this function before, and more so now that will also do reconciliation from inside here if we deem its necessary.
We are:
Checking to see if we need to do live repair, and possibly checking to see if we need to do reconciliation.
Maybe call this verify_downstairs_consistency()?
I'm open to other suggestions too.
The comment where we call this should be updated to indicate that we are cheking for LR or Reconciliation.
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.
Hmmm, it also starts live-repair, so just verify isn't great (neither is check for that matter).
What about ensure_downstairs_consistency?
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.
Yesssssssssssss.
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.
Renamed, and comments are now updated (please let me know if I missed any!)
b3ae34e to
5094764
Compare
5094764 to
f452123
Compare
This is staged on top of the merge of #1783 and #1777, so it's hard to review right now; once those PRs are merged, I'll rebase this one.Fixes #1556 by adding a new transition from "three downstairs in LiveRepairReady" to reconciliation.
I tested this using the same sequence as #1783 to get all three downstairs faulted, then bringing them back online. Watching
upstairs_info.dshows that we successfully go through reconciliation (REC) then back to active: