support for lazy leader deletion #9025
Conversation
There was a problem hiding this comment.
Thanks for the PR 🙇 ! In general it looks good to me, I've left some minor comments.
A more general comment and something I would like to double check with @kianenigma and @Ank4n as well: I am fine with the two step approach (extending the API first, adding the code for the actual lazy deletion after). But maybe it would be nice to have both changes in a single PR.
The way I had in mind when with_data=false- but it's more of a draft idea than a properly crafted approach so very up to debate- was to introduce a new storage item for tracking page-level cleanup. take_leader_with_data with with_data=false, adds pages to cleanup queue. And on the on_initialize() of the signed pallet, we do the cleanup maybe one page per block or something like that, to minimize load spreading.
| @@ -0,0 +1,8 @@ | |||
| title: EPMB / signed pallet: Add support for lazy deletion of the leader from the storage | |||
There was a problem hiding this comment.
| title: EPMB / signed pallet: Add support for lazy deletion of the leader from the storage | |
| title: "EPMB / signed pallet: Add support for lazy deletion of the leader from the storage" |
| Lazy deletion of leader from storage | ||
| crates: | ||
| - name: pallet-election-provider-multi-block | ||
| bump: minor |
There was a problem hiding this comment.
probably major since we have changed a pub(crate) API but CI will tell us
| pub(crate) fn take_leader_with_data( | ||
| /// If `with_data` is `true`, it will also remove all associated submission pages. | ||
| /// If `false`, the submission pages are left in storage for later. | ||
| pub(crate) fn take_leader( |
There was a problem hiding this comment.
it would be probably nice to have tests covering both cases, verifying that in one case submission pages are deleted and in the other cases are left where they are.
| pub(crate) fn take_leader_with_data( | ||
| /// If `with_data` is `true`, it will also remove all associated submission pages. | ||
| /// If `false`, the submission pages are left in storage for later. | ||
| pub(crate) fn take_leader( |
There was a problem hiding this comment.
[nitpick] please rebase , after #7997 has been merged this fn is now called take_leader_with_data. You might also want to do a round of fmt to make CI happy (I would suggest you to use something like cargo +nightly-2025-01-24 fmt --all locally or the the /cmd fmt bot)
|
This is a good addition, but I would not address it right away as we have other priorities. What I see crucially missing so far is any test demonstrating the e2e flow. |
3 participants
closes #8719