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: Alerter backup saving #335

Merged
merged 9 commits into from
Aug 29, 2023
Merged

A0-544: Alerter backup saving #335

merged 9 commits into from
Aug 29, 2023

Conversation

woocash2
Copy link
Contributor

@woocash2 woocash2 commented Aug 28, 2023

Backup saving is added into alerter::Service as an intermediary step between handling the data by Handler, and sending the data out to the network so that the cycle of data of type Alert or Multisigned which passes through alerter is like this:

  • data gets handled by the Handler which returns some response which is stored in the Service
  • data is passed to backup via an appropriate channel
  • after some time backup responds with data which triggers a branch in the main select! loop
  • response corresponding to data is retrieved from the Service's storage
  • parts of response is sent through the network (or we do some rmc stuff with it)

Heavy parts:

  • refactor Handler so that there is no on_message(message) but it's split into a number of functions dependent on which variant is the message
  • refactor Service so that match message happens inside handle_network_alert and not in on_message in Handler (which no longer exists).

This is done so that backup save can be triggered inside Service and not handler (it needs to happen only in a specific variant of message).

@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

Copy link
Collaborator

@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 work, only two tiny comments, you might even want to just ignore the latter if it's too much work.

consensus/src/alerts/service.rs Outdated Show resolved Hide resolved
consensus/src/alerts/service.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kostekIV kostekIV left a comment

Choose a reason for hiding this comment

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

One comment, possibly ignorable

consensus/src/alerts/service.rs Show resolved Hide resolved
@woocash2 woocash2 merged commit 1b5e7b2 into main Aug 29, 2023
11 checks passed
@woocash2 woocash2 deleted the A0-544-alerts-backup-saving branch August 29, 2023 12:19
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