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.

Sign up for GitHub

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

Jump to bottom

feat(stages): sender recovery stage progress #2910

Merged
merged 6 commits into from
Jun 1, 2023
Merged

feat(stages): sender recovery stage progress #2910

merged 6 commits into from
Jun 1, 2023

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented May 30, 2023 •
edited
Loading

Continuing with the same idea as in #2820, we calculate progress for Sender Recovery stage based on the number of entries in the TxSenders table vs number of entries in the Transactions table. Both contain only canonical transactions, so it should be correct.

@onbjerg onbjerg added C-enhancement New feature or request A-staged-sync Related to staged sync (pipelines and stages) labels May 30, 2023
@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch 2 times, most recently from edf3394 to af6663d Compare May 30, 2023 10:29
@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch from af6663d to b8820bc Compare May 30, 2023 11:02
@shekhirin shekhirin marked this pull request as ready for review May 30, 2023 11:02
rkrasiuk
rkrasiuk reviewed May 30, 2023
View reviewed changes
crates/stages/src/stages/sender_recovery.rs Outdated
@@ -145,8 +149,18 @@ impl<DB: Database> Stage<DB> for SenderRecoveryStage {
}
}

// Drop cursors, so we can get mutable tx borrow for stage progress calculation
Copy link
Member

Choose a reason for hiding this comment

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

why does it need to be mut?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops there's actually no need, fixed in other stages too

@shekhirin shekhirin requested a review from rkrasiuk May 30, 2023 12:35
rkrasiuk
rkrasiuk reviewed May 30, 2023
View reviewed changes
crates/stages/src/stages/sender_recovery.rs Outdated
}
}

fn stage_progress<DB: Database>(
Copy link
Member

Choose a reason for hiding this comment

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

should we define this on the trait level once we implement it for all of the stages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure because only 3 stages use this approach with calculating total entries in different tables. For others, we increment/decrement processed according to changes in the stage execution/unwind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what definition on trait level do you have in mind? maybe I misunderstood

Copy link
Member

Choose a reason for hiding this comment

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

just in general, every stage must implement stage_progress/stage_checkpoint method. the implementation details are up to the respective stage

@shekhirin shekhirin requested a review from rkrasiuk May 31, 2023 12:10
@shekhirin shekhirin requested a review from gakonst as a code owner May 31, 2023 15:37
@shekhirin shekhirin removed the request for review from gakonst May 31, 2023 15:41
@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch from 760fd23 to 0926895 Compare May 31, 2023 16:02
@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch 2 times, most recently from f22a694 to c91dbe8 Compare May 31, 2023 16:15
@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch from c91dbe8 to 12ae48a Compare May 31, 2023 16:22
@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch from 12ae48a to 3879a36 Compare May 31, 2023 16:30
rkrasiuk
rkrasiuk approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

lgtm overall, shall we get rid of progress terminology all together? let's rename the stage_progress fns to stage_checkpoint

@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch from 073b209 to b0027b8 Compare June 1, 2023 09:35
@shekhirin
Copy link
Collaborator Author

@rkrasiuk sg. In hashing stages, renamed to stage_checkpoint_progress, because it doesn't return full checkpoint, but rather the progress portion of it.

@shekhirin shekhirin force-pushed the alexey/sender-recovery-progress branch from 569a215 to 1b43946 Compare June 1, 2023 12:24
@shekhirin shekhirin merged commit 57cd8aa into feat/checkpoints Jun 1, 2023
@shekhirin shekhirin deleted the alexey/sender-recovery-progress branch June 1, 2023 13:13
@shekhirin shekhirin mentioned this pull request Jun 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkrasiuk rkrasiuk rkrasiuk approved these changes

@rakita rakita Awaiting requested review from rakita

@joshieDo joshieDo Awaiting requested review from joshieDo

@onbjerg onbjerg Awaiting requested review from onbjerg

Assignees
No one assigned
Labels
A-staged-sync Related to staged sync (pipelines and stages) C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shekhirin @rkrasiuk @onbjerg