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

A0-544: Prepare alerts backup #329

Merged
merged 10 commits into from
Aug 28, 2023
Merged

A0-544: Prepare alerts backup #329

merged 10 commits into from
Aug 28, 2023

Conversation

woocash2
Copy link
Contributor

@woocash2 woocash2 commented Aug 22, 2023

This pr mainly refactors runway/backup.rs so that it accounts for different items in backup than only units.
No saving backup in alerter yet.

@github-actions
Copy link

Please make sure the following happened

  • Appropriate tests created
  • Infrastructure updated accordingly
  • Updated existing documentation
  • New documentation created
  • Version bumped if breaking changes

@woocash2 woocash2 requested review from timorl and fixxxedpoint and removed request for timorl August 22, 2023 09:30
@woocash2 woocash2 changed the title Prepare alerts backup A0-544: Prepare alerts backup Aug 22, 2023
Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

Nice, mostly notation comments, except the one about typing of BackupItem.

consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/mod.rs Outdated Show resolved Hide resolved
consensus/src/runway/mod.rs Outdated Show resolved Hide resolved
consensus/src/runway/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@timorl timorl left a comment

Choose a reason for hiding this comment

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

Nice, feel free to resolve the last comment and either do this in a followup PR or just completely ignore it if it actually isn't helpful.

consensus/src/runway/backup.rs Show resolved Hide resolved
Copy link
Contributor

@fixxxedpoint fixxxedpoint left a comment

Choose a reason for hiding this comment

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

LGTM. Left few small requests (address them) and suggestions/overall complaints about this part of AlephBFT (just consider them and resolve by yourself...).

consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
consensus/src/runway/mod.rs Outdated Show resolved Hide resolved
The best I could do to keep `run` as the only async function...
consensus/src/runway/backup.rs Outdated Show resolved Hide resolved
@woocash2 woocash2 merged commit 5a64132 into main Aug 28, 2023
10 checks passed
@woocash2 woocash2 deleted the A0-544-alerts-backup-prep branch August 28, 2023 16:27
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.

3 participants