Skip to content

pallet-scheduler: Put back postponed tasks into the agenda#7790

Merged
bkchr merged 11 commits intomasterfrom
bkchr-scheduler-postponed
Mar 5, 2025
Merged

pallet-scheduler: Put back postponed tasks into the agenda#7790
bkchr merged 11 commits intomasterfrom
bkchr-scheduler-postponed

Conversation

@bkchr
Copy link
Copy Markdown
Member

@bkchr bkchr commented Mar 3, 2025

Right now pallet-scheduler is not putting back postponed tasks into the agenda when the early weight check is failing. This pull request ensures that these tasks are put back into the agenda and are not just "lost".

Right now `pallet-scheduler` is not putting back postponed tasks into the agenda when the early weight check is failing. This
pull request ensures that these taks are put back into the agenda and are not just "lost".
@bkchr bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Mar 3, 2025
@bkchr bkchr requested a review from a team as a code owner March 3, 2025 22:27
@bkchr
Copy link
Copy Markdown
Member Author

bkchr commented Mar 3, 2025

/cmd prdoc --audience runtime-dev --bump patch

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 3, 2025

Command "prdoc --audience runtime-dev --bump patch" has failed ❌! See logs here

@bkchr
Copy link
Copy Markdown
Member Author

bkchr commented Mar 3, 2025

/cmd prdoc --audience runtime_dev --bump patch

);
if !weight.can_consume(base_weight) {
postponed += 1;
agenda[agenda_index as usize] = Some(task);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, so this is what was missing from before: the task would have been taken above, and not put back in the agenda.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Copy Markdown
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

The change looks good, but I don't understand another part of the code,

if service_task fails, we put back the task into agenda, even when error is Unavailable.

agenda[agenda_index as usize] = match result {
Err((Unavailable, slot)) => {
dropped += 1;
slot
},
Err((Overweight, slot)) => {
postponed += 1;
slot
},
Ok(()) => {
*executed += 1;
None
},
};

But in service_task we do return some task with error Unavailable when:

  • getting the preimage failed
  • the execution fails because it is overweight and it is the first task

If any of these case happen, the task will stay forever on the agenda of the block and brick the agenda no?

It feels like it should be dropped instead.

bkchr and others added 3 commits March 4, 2025 08:05
Co-authored-by: Alexandre R. Baldé <[email protected]>
@bkchr
Copy link
Copy Markdown
Member Author

bkchr commented Mar 4, 2025

If any of these case happen, the task will stay forever on the agenda of the block and brick the agenda no?

I have to admit, every time I take a close look at the scheduler, I find something weird :) However, what you said should be fine in most of the cases.

  • Unavailable pre-image could be uploaded later
  • Something not being able to be executed in the current block, doesn't mean this assumption is true in the next block, because we maybe already executed something before.

@bkchr bkchr enabled auto-merge March 4, 2025 10:25
@bkchr bkchr added this pull request to the merge queue Mar 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 4, 2025
@bkchr bkchr added this pull request to the merge queue Mar 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2025
@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13683530253
Failed job name: test-doc

@bkchr bkchr enabled auto-merge March 5, 2025 21:58
@bkchr bkchr added this pull request to the merge queue Mar 5, 2025
Merged via the queue into master with commit 00d8eea Mar 5, 2025
241 of 253 checks passed
@bkchr bkchr deleted the bkchr-scheduler-postponed branch March 5, 2025 23:42
ordian added a commit that referenced this pull request Mar 6, 2025
* master:
  pallet-scheduler: Put back postponed tasks into the agenda (#7790)
  ci: update credentials for command-backport (#7798)
  [XCM] add generic location to account converter that also works with external ecosystems (#7313)
  [AHM] Make more stuff public (#7802)
  Support adding extra request-response protocols to the node (#7708)
  notifications/tests: Fix tests to avoid overflow on adding duration to instant (#7793)
  pallet revive: rpc build script should not panic (#7786)
  pallet-bridge-relayers - migrate benchmarks to v2 (#7531)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T2-pallets This PR/Issue is related to a particular pallet.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants