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

refactor: forester: add retry logic for epoch registration #1160

Merged
merged 5 commits into from
Sep 7, 2024

Conversation

sergeytimoshin
Copy link
Contributor

Consolidate repeated active phase work processing into a new function process_epoch_work.
Introduce register_for_epoch_with_retry to handle registration retries with a specified maximum number of attempts and delay duration.

@sergeytimoshin sergeytimoshin changed the title refactor: add retry logic for registration refactor: forester: add retry logic for epoch registration Sep 6, 2024
Copy link
Contributor

@ananas-block ananas-block left a comment

Choose a reason for hiding this comment

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

Further changes are necessary to handle not only active work but subsequent epoch steps correctly for recovered epochs.

Consolidate repeated active phase work processing into a new function `process_epoch_work`. Introduce `register_for_epoch_with_retry` to handle registration retries with a specified maximum number of attempts and delay duration.
Eliminated an unnecessary info log statement when setting epoch flags.
@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-epoch-registration-retry branch from ec1cd25 to 7213266 Compare September 7, 2024 15:30
}

// Attempt to recover registration info
let mut registration_info = match self.recover_registration_info(epoch).await {
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do if there is no registration that can be recovered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return Err(ForesterError::Custom(format!(
"Too late to register for epoch {}. Current slot: {}, Registration end: {}",
epoch, slot, phases.registration.end
)));
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really want to throw in this case?
An alternative behavior could be to wait for the next registration period.
Or just return if we have logic to wait for the next epoch in a different place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this behaviour is semantically correct because it's part of the process_epoch flow: this case is really an error, it shouldn't happen in a normal situation, and we log it as an error in our logfiles: https://github.com/Lightprotocol/light-protocol/pull/1160/files/7213266f76690b8931a73e01055af9123b07b22a#diff-1e165bd3189acab768bdb1cd6cf4ec33528400511ec68498004b73a25d0646c3R147

Replaced multiple calls to slot_tracker.estimated_current_slot with sync_slot to ensure accurate slot synchronization. Updated sync_slot to return the current slot and adjusted the calling logic accordingly to maintain correct epoch phase handling.
@sergeytimoshin sergeytimoshin merged commit 44b1c05 into main Sep 7, 2024
7 checks passed
@sergeytimoshin sergeytimoshin deleted the sergey/forester-epoch-registration-retry branch September 7, 2024 17:03
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.

2 participants