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

pallet-message-queue: add queue pausing #14318

Merged
merged 3 commits into from
Jun 28, 2023
Merged

pallet-message-queue: add queue pausing #14318

merged 3 commits into from
Jun 28, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jun 7, 2023 •
edited
Loading

Introduces a QueuePausedQuery hook into the MQ pallet that is polled once before starting to service a queue. This hook decided whether the queue should start service or not.

It can be used to dynamically pause queues and is better than using Yield since no PoV is wasted for loading the first message and ProcessMessage does not need to be implemented just for pausing.
Context: Greatly reduces the complexity of paritytech/cumulus#2157

I also implemented explicit suspension (via set_suspended) by adding a bool flag to the BookState here. This is generally a nice thing to have for the future IMHO, but not well usable for 2157. This approach works better when knowing in advance which queues need to stay paused without any more complicated predicates involved.
2157 instead has custom predicate logic here which can change at any time.

Changes:

  • 🚨 Add QueuePausedQuery to the MQ pallet Config. () indicates that nothing is paused.
  • Add trait frame_support::traits::QueuePausedQuery
  • Extend pallet_message_queue::Error and ExecuteOverweightError

Polkadot companion: paritytech/polkadot#7433

@ggwpez
pallet-message-queue: add queue pausing
a3d9e58 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. C1-low PR touches the given topic and has a low impact on builders. labels Jun 7, 2023
@ggwpez
Fix build
76fb2b5 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added the E6-needs_polkadot_pr This is an issue that needs to be implemented upstream in Polkadot. label Jun 7, 2023
gavofyork
gavofyork approved these changes Jun 7, 2023
View reviewed changes
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.

Seem harmless enough, but what about migrations?

@ggwpez
Copy link
Member Author

ggwpez commented Jun 7, 2023

Seem harmless enough, but what about migrations?

Its not changing any storage - just adding a QueuePausedQuery hook.

@ggwpez ggwpez marked this pull request as ready for review June 7, 2023 14:33
@ggwpez ggwpez requested review from a team June 7, 2023 14:33
@ggwpez ggwpez 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 Jun 7, 2023
@kianenigma kianenigma self-requested a review June 8, 2023 06:59
kianenigma
kianenigma reviewed Jun 8, 2023
View reviewed changes
frame/support/src/traits/messages.rs
@@ -220,3 +230,15 @@ where
E::footprint(O::get())
}
}

/// Provides information on paused queues.
pub trait QueuePausedQuery<Origin> {
Copy link
Contributor

Choose a reason for hiding this comment

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

As a design nit, I sometimes wonder if we should avoid creating a new trait like this when it can be easily expressed by Convert<Origin, bool> (cc @gpestana this is similar to my comment in #13983 (comment) 🙈).

No strong opinion myself, but I might personally lean into reusing Convert more and more.

Copy link
Member Author

@ggwpez ggwpez Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting idea. Yes I also dont like having tons of trivial traits.

The only problem i see with Convert is on the implementor side. When seeing impl<T: Config> Convert… for Pallet<T>, it is not clear what it means. Maybe we can use type alias for that, or something similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use the frame_support::Get trait instead? The type name is explicitly enough to use the getter trait and it things readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suppose we have impl Get<bool> for Pallet<T>.
Now from the implementor side what does this mean? Anyone seeing this code will have no idea what it means.

kianenigma
kianenigma approved these changes Jun 8, 2023
View reviewed changes
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.

LGTM. I would suggest double checking that all code-paths leading into servicing queues is blocked, as I could not be sure that the places that are already checked are not missing anything.

@ggwpez
Remove check
add9382 
Otherwise it would not start servicing queues that started paused
and became unpaused afterwards.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez mentioned this pull request Jun 8, 2023
Open
10 tasks
gpestana
gpestana approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@gpestana gpestana left a comment

Choose a reason for hiding this comment

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

Added a question and a suggestion wrt to the QueuePausedQuery trait, but lgtm!

frame/support/src/traits/messages.rs
@@ -220,3 +230,15 @@ where
E::footprint(O::get())
}
}

/// Provides information on paused queues.
pub trait QueuePausedQuery<Origin> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use the frame_support::Get trait instead? The type name is explicitly enough to use the getter trait and it things readable.

frame/support/src/traits/messages.rs Show resolved Hide resolved
@louismerlin louismerlin added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 27, 2023
@ggwpez ggwpez mentioned this pull request Jun 27, 2023
Merged
@ggwpez
Copy link
Member Author

ggwpez commented Jun 28, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit a84f0af into master Jun 28, 2023
@paritytech-processbot paritytech-processbot bot deleted the oty-mq-pause-check branch June 28, 2023 08:58
@svyatonik svyatonik mentioned this pull request Jul 12, 2023
Closed
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@ggwpez @nathanwhit
pallet-message-queue: add queue pausing (paritytech#14318)
8d7d178 
* pallet-message-queue: add queue pausing

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

* Fix build

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

* Remove check

Otherwise it would not start servicing queues that started paused
and became unpaused afterwards.

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

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gupnik gupnik gupnik left review comments

@gavofyork gavofyork gavofyork approved these changes

@gpestana gpestana gpestana approved these changes

@sam0x17 sam0x17 sam0x17 approved these changes

@kianenigma kianenigma kianenigma approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited E6-needs_polkadot_pr This is an issue that needs to be implemented upstream in Polkadot. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ggwpez @gavofyork @gpestana @sam0x17 @kianenigma @gupnik @louismerlin