Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Scheduler: remove empty agenda on cancel #12989

Merged
merged 7 commits into from
Dec 21, 2022

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Dec 21, 2022

Fixes #12977 #12851

With the small change to the script provided by @ggwpez, you can see there are only agendas for the future blocks with all None items.
Its possible because we do not remove such agendas on a cancelation, and they get removed when serviced.

From the implementation and tests it looks like it was expected to have empty future agendas, nevertheless it seems to me valid, to remove them on the cancelations and address the raised issues.

// DONE fix benches
// DONE migration to remove empty agendas

@muharem muharem added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Dec 21, 2022
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

It seems like the original author intended to be like this, but seeing the data I also agree that we better not have all of these empty agendas.

I don't think we need a migration, the existing agendas would eventually be cleared up when they are in the past, right?

@muharem
Copy link
Contributor Author

muharem commented Dec 21, 2022

It seems like the original author intended to be like this, but seeing the data I also agree that we better not have all of these empty agendas.

I don't think we need a migration, the existing agendas would eventually be cleared up when they are in the past, right?

Yes, looks same to me.
And yes, they will be eventually removed.

@muharem muharem added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Dec 21, 2022
@ggwpez
Copy link
Member

ggwpez commented Dec 21, 2022

With the small change to the script provided by @ggwpez, you can see there are only agendas for the future blocks with all None items.

Hm can you share the modification? For me it looks like about 1/3 of them are in the past. So either a manual cleanup call or a migration will still be needed.
PS: Yes you are right.

frame/scheduler/src/lib.rs Outdated Show resolved Hide resolved
@muharem
Copy link
Contributor Author

muharem commented Dec 21, 2022

With the small change to the script provided by @ggwpez, you can see there are only agendas for the future blocks with all None items.

Hm can you share the modification? For me it looks like about 1/3 of them are in the past. So either a manual cleanup call or a migration will still be needed. PS: I will add one.

Only extended the condition with if block < block_number and agenda.value.count(None) == len(agenda.value):

muharem and others added 2 commits December 21, 2022 13:46
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member

ggwpez commented Dec 21, 2022

Only extended the condition with if block < block_number and agenda.value.count(None) == len(agenda.value):

But the current block is last_block = substrate.get_block_header()['header']['number'].
The block_number is the the snapshot position at which it requests the state, not the agendas block.

Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Looks good, but CI needs to pass still.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@muharem
Copy link
Contributor Author

muharem commented Dec 21, 2022

Bot merge

@paritytech-processbot paritytech-processbot bot merged commit 5aaf5f4 into master Dec 21, 2022
@paritytech-processbot paritytech-processbot bot deleted the muharem-scheduler-empty-agenda branch December 21, 2022 15:25
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1

ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
* Scheduler: remove empty agenda on cancel

* use iter any

* fix benches

* remove trailing None

* Add CleanupAgendas migration

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fix ci

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Count non-empty agendas in migration

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Scheduler: remove empty agenda on cancel

* use iter any

* fix benches

* remove trailing None

* Add CleanupAgendas migration

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fix ci

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Count non-empty agendas in migration

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup empty scheduler agendas
5 participants