Skip to content

Remove periodic from scheduler#404

Merged
n13 merged 5 commits intomainfrom
remove_periodic_schedules
Mar 10, 2026
Merged

Remove periodic from scheduler#404
n13 merged 5 commits intomainfrom
remove_periodic_schedules

Conversation

@n13
Copy link
Collaborator

@n13 n13 commented Mar 9, 2026

Remove Periodic

  • We don't need periodic, but it complicates the scheduler
  • update all calls, remove all periodic tests, benchmarks

update all calls, remove all periodic tests
@n13 n13 changed the title remove periodic from scheduler Remove periodic from scheduler Mar 9, 2026
@n13
Copy link
Collaborator Author

n13 commented Mar 9, 2026

Gemini review

The changes look fantastic and perfectly achieve the goal of removing the periodic feature to simplify the scheduler. Here is a summary of my review:

Completeness: I verified that all traces of periodic logic (like the Period type, maybe_periodic parameters in schedule functions, and weight configs) have been cleanly removed from the scheduler primitives, traits, and pallets.
Dependencies: All external calls to schedule across the codebase (such as in pallets/reversible-transfers and runtime/tests) were properly updated to omit the removed periodic argument.
Tests: I checked the test results from your local pre_checkin_checks.sh run and everything compiles cleanly, tests pass, and there are no Clippy warnings! 🎉
While reviewing, I did spot a few lingering references to "periodic" in the documentation comments within pallets/scheduler/src/lib.rs and the README.md. I have proactively cleaned these up in your local workspace!

Here is what I cleaned up in the doc comments locally:

Removed which may be periodic from pallets/scheduler/README.md
Removed mentions of non-periodic and periodic retrying logic in the comments for as_retry, set_retry, set_retry_named, and service_task inside pallets/scheduler/src/lib.rs.
The PR is in great shape! Let me know if you would like me to commit these small doc updates and push them to your branch on GitHub! 🚀

^^ these changes were committed... no more periodic in comments

@n13
Copy link
Collaborator Author

n13 commented Mar 10, 2026

Notes

There's some annoying v3:: scheduler traits defined in this class

These are used in pallet_referenda, they are defined in frame_support

So we could delete them if we get rid of or fork pallet referenda.

@n13
Copy link
Collaborator Author

n13 commented Mar 10, 2026

v3:: functions usage

pallet_referenda is the only consumer of v3::Anon and v3::Named. It uses exactly 3 methods:

  • schedule(DispatchTime::At(..), None, priority, origin, call) — one-off alarm scheduling
  • schedule_named(name, DispatchTime::At(..), None, priority, origin, call) — enactment scheduling
  • cancel(address) — 4 call sites cancelling alarms

Periodic is never used. Every call passes None for maybe_periodic. No code in the entire workspace ever passes a non-None periodic value.

The path to removing v3: Since pallet-referenda is an external crate from crates.io (v45.0.0), you'd need to fork it locally. The change inside it would be small -- just swap the trait bounds from v3::Anon/v3::Named to your own qp_scheduler traits, and drop the None periodic argument from 2 call sites.

The plan would be:

  1. Add a ScheduleAnon trait to qp-scheduler primitives (like your existing ScheduleNamed but anonymous)
  2. Copy pallet-referenda into your local pallets as a fork
  3. Update its Config to use qp_scheduler::{ScheduleAnon, ScheduleNamed} instead of v3::{Anon, Named}
  4. Implement ScheduleAnon on the scheduler pallet
  5. Remove the v3 impls and map_err_to_v3_err

This touches several crates and is a meaningful refactor. Want me to go ahead with this, or would you rather keep the v3 impls as thin shims for now? They're only ~50 lines and they just delegate to your internal methods.

n13 added 2 commits March 10, 2026 10:28
crash on periodic from the v3 shims (never invoked by our current deps)
@n13 n13 requested review from czareko and illuzen March 10, 2026 02:37
@n13 n13 merged commit 948c452 into main Mar 10, 2026
4 checks passed
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