Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sweep expired tasks, fixed #442 #451

Merged
merged 10 commits into from
Oct 12, 2023
Merged

Sweep expired tasks, fixed #442 #451

merged 10 commits into from
Oct 12, 2023

Conversation

v9n
Copy link
Member

@v9n v9n commented Oct 10, 2023

When block has idle time, we will run task sweep to delete expired tasks. We will run as long as we still have weight left leverage on_idle block https://paritytech.github.io/polkadot-sdk/master/frame_support/traits/trait.Hooks.html#method.on_idle

I had a few test cases to test this extensively. When removing tasks, we also update our stat such as total task count and task count of user that has task being removed.

@v9n v9n changed the base branch from master to handle-price-move-before-task-run October 10, 2023 16:19
@v9n v9n marked this pull request as draft October 10, 2023 16:19
@v9n v9n changed the title Sweep expired task Sweep expired task, fix #442 Oct 10, 2023
@v9n v9n changed the title Sweep expired task, fix #442 Sweep expired task, fixed #442 Oct 10, 2023
@v9n v9n added the WIP work in progress, not ready for review label Oct 10, 2023
@v9n v9n force-pushed the handle-price-move-before-task-run branch from 5c41a00 to d59d488 Compare October 11, 2023 15:32
The price may have changed in extreme case by the time we run the tasks,
if so, we adjust accordingly and emit an event.

Task won't be put back to the queue but will be removed from the queue
as if it has run succesfully, but we emit the event about stale price
instead of executing the task
@v9n v9n changed the base branch from handle-price-move-before-task-run to master October 12, 2023 03:11
@v9n v9n marked this pull request as ready for review October 12, 2023 08:59
@v9n v9n changed the title Sweep expired task, fixed #442 Sweep expired tasks, fixed #442 Oct 12, 2023
@v9n v9n removed the WIP work in progress, not ready for review label Oct 12, 2023
@v9n v9n self-assigned this Oct 12, 2023
@v9n v9n requested a review from chrisli30 October 12, 2023 09:52
@v9n v9n requested a review from imstar15 October 12, 2023 10:07
@v9n
Copy link
Member Author

v9n commented Oct 12, 2023

Hi @chrisli30 @imstar15 This is ready to review. Please help check. Thanks.

// TaskStats: decrease task count
// AccountStats: decrease task count
// SortedTasksIndex
pub fn remove_task(task: &Task<T>, event: Option<Event<T>>) {
Copy link
Member

Choose a reason for hiding this comment

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

Yea definitely needs this encapsulation of remove tasks.

// TaskQueue if the task is alreayd queue
// TaskStats: decrease task count
// AccountStats: decrease task count
// SortedTasksIndex
Copy link
Member

Choose a reason for hiding this comment

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

Let’s add comments.

SortedTasksIndex - sorted by price
Sorted TaskIds by expiration?

Copy link
Member Author

Choose a reason for hiding this comment

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

comment are added now.

BTreeMap::<u128, TaskIdList<T>>::new()
}
#[pallet::storage]
#[pallet::getter(fn get_sorted_tasks_by_expiration)]
Copy link
Member Author

@v9n v9n Oct 12, 2023

Choose a reason for hiding this comment

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

@chrisli30 This is the SortedTaskByExpiration where we store a tasks sorted by expired epoch

@v9n v9n merged commit b9c357e into master Oct 12, 2023
3 checks passed
@v9n v9n deleted the sweep-expired-task branch October 12, 2023 22:10
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