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

fix pruner timing issue with batch pruning #14929

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

fix pruner timing issue with batch pruning #14929

wants to merge 9 commits into from

Conversation

dB2510
Copy link
Contributor

@dB2510 dB2510 commented Feb 13, 2025

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?
This PR fixes a bug in pruner where it was taking a lot of time to prune a large database therefore blocking everything else because boltDB locks the database until pruning is completed.

Other notes for review
Solved the above by pruning in batches.

Acknowledgements

@dB2510 dB2510 requested a review from a team as a code owner February 13, 2025 20:34
@dB2510 dB2510 requested review from kasey, nalepae and nisdas February 13, 2025 20:34
@@ -336,7 +325,7 @@ func (s *Store) SaveBlock(ctx context.Context, signed interfaces.ReadOnlySignedB
// if a `saveBlindedBeaconBlocks` key exists in the database. Otherwise, we check if the last
// blocked stored to check if it is blinded, and then write that `saveBlindedBeaconBlocks` key
// to the DB for future checks.
func (s *Store) shouldSaveBlinded(ctx context.Context) (bool, error) {
func (s *Store) shouldSaveBlinded() (bool, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this ctx variable as it was unused

@@ -159,6 +169,24 @@ func (p *Service) prune(slot primitives.Slot) error {
return nil
}

func (p *Service) pruneBatches(pruneUpto primitives.Slot) (int, error) {
ctx, cancel := context.WithDeadline(p.ctx, time.Now().Add(defaultPruningWindow))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ctx, cancel := context.WithDeadline(p.ctx, time.Now().Add(defaultPruningWindow))
ctx, cancel := context.WithTimeout(p.ctx, defaultPruningWindow)

A little easier this way and achieves the same result

Comment on lines +177 to +180
for {
select {
case <-ctx.Done():
return numBatches, nil
Copy link
Member

Choose a reason for hiding this comment

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

Does this routine always way until the context has expired?

What happens when there is nothing to do? Seems like this routine should exit the loop early if there are no batches left to prune.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I haven't mentioned the number of batches to prune. The idea is to prune as many batches in 3 seconds.

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