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

av-store: Move prune on a separate thread #7263

Merged
merged 3 commits into from
Jun 8, 2023
Merged

av-store: Move prune on a separate thread #7263

merged 3 commits into from
Jun 8, 2023

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented May 22, 2023 •
edited
Loading

There are situations where pruning of the data could take more than a few seconds and that might make the whole subsystem unreponsive. To avoid this just move the prune process on a separate thread.

See: #7237, for more details.

@alexggh alexggh changed the title av-store: Move prune on a separte thread av-store: Move prune on a separate thread May 22, 2023
@alexggh alexggh force-pushed the feature/my_first_commit branch 2 times, most recently from 7ff883b to f34c93f Compare May 23, 2023 09:22
@alexggh alexggh added the T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance. label May 23, 2023
@alexggh alexggh requested a review from sandreim May 23, 2023 09:24
@alexggh alexggh marked this pull request as ready for review May 23, 2023 09:42
@alexggh alexggh mentioned this pull request May 23, 2023
Closed
@alexggh alexggh self-assigned this May 23, 2023
@alexggh alexggh added the C1-low PR touches the given topic and has a low impact on builders. label May 23, 2023
@sandreim sandreim 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. and removed C1-low PR touches the given topic and has a low impact on builders. labels May 23, 2023
sandreim
sandreim approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

Nice! Just one nit.

node/core/av-store/src/lib.rs
ctx.spawn_blocking(
"av-store-prunning",
Box::pin(async move {
let _timer = metrics.time_pruning();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd also add a debug! log here to know when pruning starts and ends. It would be nice to have when debugging.

Copy link
Contributor Author

@alexggh alexggh May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yep will do.
Done

ordian
ordian approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@ordian ordian 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 I would like to see a Versi burn-in to make sure it doesn't break anything. Although I don't see why it would.

@sandreim
Copy link
Contributor

Looks good, but I would like to see a Versi burn-in to make sure it doesn't break anything. Although I don't see why it would.

Let’s burn it in and observe the pruning log and column usage. I also agree that this cannot break anything.

@alexggh
Copy link
Contributor Author

alexggh commented May 24, 2023

Looks good, but I would like to see a Versi burn-in to make sure it doesn't break anything. Although I don't see why it would.

Let’s burn it in and observe the pruning log and column usage. I also agree that this cannot break anything.

Yeah, makes sense, I'll let you know once I have the results.

mrcnski
mrcnski approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Nice!

node/core/av-store/src/lib.rs
Comment on lines +581 to +584
(pruning_result_tx, pruning_result_rx): (
&mut MpscSender<Result<(), Error>>,
&mut MpscReceiver<Result<(), Error>>,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It would be simpler for this not to be a tuple, and it would save two lines. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean just to have two separate params pruning_result_tx and pruning_result_rx ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Just a style nit, feel free to ignore if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will keep it as it is than :) , since I think it makes sense to try to have closely related params in the same data structure.

@alexggh
av-store: Move prune on a separate thread
039caa1 
There are situations where pruning of the data could take more than a few
seconds and that might make the whole subsystem unreponsive. To avoid this just
move the prune process on a separate thread.

See: paritytech#7237, for more details.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh
av-store: Add log that prunning started
140ab8e 
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh
av-store: modify log severity
0dde5a9 
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh
Copy link
Contributor Author

alexggh commented Jun 8, 2023

Looks good, but I would like to see a Versi burn-in to make sure it doesn't break anything. Although I don't see why it would.

Let’s burn it in and observe the pruning log and column usage. I also agree that this cannot break anything.

Ran this in versi for more than 24h, pruning was starting and stopping as expected. Looking at the data for database usage on col0 which is what av-store is using we can see the memory usage remains constant even with this PR.

Screenshot 2023-06-08 at 12 54 05

So, I'm going to go ahead and merge this PR.

@sandreim
Copy link
Contributor

sandreim commented Jun 8, 2023

Nice! Thanks @alexggh

@alexggh
Copy link
Contributor Author

alexggh commented Jun 8, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b1cc6fa into paritytech:master Jun 8, 2023
@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

@mrcnski mrcnski mrcnski approved these changes

@ordian ordian ordian approved these changes

@sandreim sandreim sandreim approved these changes

Assignees

@alexggh alexggh

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. T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexggh @sandreim @ordian @mrcnski